Skip to content

CLI cleanup: split core_commands, unify argv parsing, delete the dead walkthrough - #241

Merged
platypii merged 7 commits into
masterfrom
cli-cleanup
Jul 5, 2026
Merged

CLI cleanup: split core_commands, unify argv parsing, delete the dead walkthrough#241
platypii merged 7 commits into
masterfrom
cli-cleanup

Conversation

@platypii

Copy link
Copy Markdown
Contributor

Option C from the complexity review, plus the kernel-types rename requested along the way. Six commits, each reviewable on its own:

  1. Delete the dead legacy walkthrough (−300). runWalkthrough/composeConfig had no production callers since the picker replaced them; the one smoke flow that drove the legacy path headlessly (only to validate the span contract) now drives the picker and asserts the picker's span shape.

  2. Split core_commands.js into per-topic modules (4,597 → 370 lines). Implementations move verbatim to src/core/commands/{status,query,plugin,config,daemon,mcp,misc,sink,init,central,clients}.js, next to the existing backfill.js; core_commands.js keeps the registry table. This also breaks the core_commandsremote_commands import cycle (remote_commands now reaches enrollCentralSink via commands/central.js). Five imports referenced only in comments/usage strings were dropped; parseMcpArgv is no longer exported (no users).

  3. Test-fixture fix: the config-apply-deps git fixture now disables tag/commit signing so a host-level tag.gpgsign=true cannot break npm test.

  4. Unify argv parsing on the verb codec (−139 net). Eleven commands hand-rolled their own argv loops with divergent --flag=value support, error wording, and help handling. New parseCommandArgv() wraps the existing schema-driven argvToParams with the two command conventions verbs don't need: -h/--help detection and short-alias expansion. Two backwards-compatible codec generalizations: greedy array positionals (hyp backfill claude codex) and items.enum. Migrated: query maintain, sink maintain, plugin install/update, config validate, daemon install/run, mcp, ignore, skills, join, leave, init, backfill run/plan. Kept bespoke: attach/detach's client parser (positional-or-flag merge with consistency check) and the five plugin-internal parsers (need a public codec export; follow-up).

  5. Dedupe stray helpers (−51). safeOrigin existed three times (canonical originOf now exported from gateway_seed); readAllStdin/isObject in remote_commands replaced by the shared stdio.js/isPlainObject; duplicated copyDir moved to src/core/util/fs_copy.js.

  6. Rename collectivus-plugin-kernel-types.d.tshypaware-plugin-kernel-types.d.ts (264 files, mechanical). The header itself said the name was only pending the project rename; no public import specifier contained it. Tombstoned LLPs and prose about collectivus-the-project untouched.

Behavior deltas (flagged deliberately)

  • Every migrated command now accepts --flag=value uniformly (several previously accepted only the space form).
  • Parse-error texts unify on the codec's wording, e.g. unknown flag --x (previously a mix of unknown flag '--x', unknown argument: --x, flag --x requires a value). Two test assertions updated to match.
  • --help anywhere in argv now wins over parse errors (previously only when it preceded the bad token). hyp plugin install --help now prints usage instead of treating --help as a source name.
  • hyp init --client claude,codex (comma form) now works alongside the repeated-flag form.

Verification

  • typecheck clean; npm test: 1829 tests, 1828 pass, 0 fail, 1 pre-existing skip; npm run build:types clean.
  • Smoke: walkthrough_picker_to_first_query*, claude_attach_detach, daemon_foreground_start_stop, plugin_install_local_dir, backfill_codex_fixture, cache_lifecycle_maintenance, mcp_stdio_tools, status_diagnostics, remote_oidc_login.

* walkthrough_picker_to_first_query and walkthrough_to_first_query fail identically on master (pre-existing, reproducible back to v1.10.0: "real hyp init attach exited 0 → 1" / an ai_gateway_messages sql row assertion). With the unrelated assertion locally relaxed, the walkthrough flow passes end-to-end including this PR's new picker span assertions. Worth picking up with Option D (smoke suite).

runPickerWalkthrough replaced it as the only production entry point;
runWalkthrough, composeConfig, and the three option builders had no
remaining callers except one smoke flow that drove the legacy path
headlessly purely to validate the span contract. That flow now drives
the picker with pre-baked picks and asserts the picker's span shape
(export_picked instead of sinks_picked, sources/exports pick logs).
Note: walkthrough_to_first_query currently fails on master for an
unrelated reason (ai_gateway_messages sql assertion, reproducible back
to v1.10.0); with that single assertion relaxed locally the flow
passes end-to-end including the new picker span assertions.
src/core/cli/core_commands.js was 4,597 lines holding every core
command implementation. It is now a 370-line registry table; the
implementations move (verbatim) to src/core/commands/, next to the
existing backfill.js:
status, query, plugin, config, daemon, mcp, misc (version+smoke),
sink, init, central (join/leave/enroll), clients (attach/detach/
ignore/skills/agents)
isTty/readAllStdin, shared by several modules, move to
src/core/cli/stdio.js.
This also breaks the core_commands <-> remote_commands import cycle:
remote_commands now imports enrollCentralSink from commands/central.js,
which reaches runDaemonInstall via commands/daemon.js instead of back
through the registry file.
Only mechanical changes beyond the moves: imports recomputed per
module (five imports referenced only in comments/usage strings were
dropped: validateConfig, collectHypAwareStatus, process,
readObservabilityEnv, path), parseMcpArgv is no longer exported (no
external users), and the four test files that imported render/init
helpers from core_commands.js now import them from their new homes.
A global tag.gpgsign=true turns the fixture's lightweight
`git tag v0.1.0` into a signed tag, which fails with 'no tag
message?'. Disable tag/commit signing for fixture git invocations.
verb_codec.js is documented as the single place CLI parsing lives, but
eleven commands still hand-rolled their own argv loops, each with its
own flag forms (--f=v support varied), error wording, and help
handling. New parseCommandArgv() wraps argvToParams with the two
command conventions the verb family does not need: -h/--help detection
(returns { help: true }) and short-alias expansion ({ '-y': '--yes' }).
Two small codec generalizations, both backwards compatible: a greedy
positional with type 'array' collects the remaining tokens as items
(hyp backfill claude codex), and array items now honor items.enum.
Migrated: query maintain, sink maintain, plugin install/update, config
validate, daemon install/run, mcp, ignore/skills, join/leave, init,
backfill run/plan (backfill's parseFlag helper deleted). Per-command
rules that are not shape (mutual exclusions, --json requires --dry-run,
join's '-' stdin sentinel, date-window validation) stay in the
commands.
Kept bespoke: parseClientArgs (attach/detach's client-from-positional-
or-flag merge with consistency check does not fit a schema) and the
five plugin-internal parsers (reaching the codec from the plugin
workspace needs a public export; follow-up).
Behavior deltas: every migrated command now accepts --flag=value
uniformly, error texts unify on the codec's wording (two test
assertions updated), and --help anywhere in argv now wins over parse
errors instead of only when it appears first.
- safeOrigin existed three times (gateway_seed's private originOf plus
copies in the join/enroll commands and remote_commands, each
annotated 'mirrors gateway_seed'). gateway_seed.js now exports
originOf and both CLI callers import it.
- readAllStdin: remote_commands' copy replaced by cli/stdio.js's.
- isObject in remote_commands was isPlainObject from hypaware/core/util
with a looser return type; the shared one drops in cleanly.
- copyDir was duplicated verbatim between the skills/agents installers
and the walkthrough finale; it moves to src/core/util/fs_copy.js
(re-exported from hypaware/core/util).
…-types.d.ts
The filename was a self-acknowledged leftover from before the project
rename ('Filename is ... only because the project rename to HypAware is
still pending'); collectivus is deprecated and the contents have tracked
the HypAware design since the initial commit. No public import specifier
contained the old name (plugins reach the file by relative path; package
consumers go through the exports map), so this is a repo-internal rename:
the file, ~230 @import paths, tsconfig/package.json entries, and doc
links in README, AGENTS.md, PLUGIN_AUTHORING, and the living LLPs.
Tombstoned LLPs and prose mentions of collectivus-the-project are
untouched.
@platypii

Copy link
Copy Markdown
ContributorAuthor

Code review — CLI cleanup

Overall: clean, low-risk refactor. The bulk is a verbatim code move (core_commands.js → per-topic modules) plus an argv-parser migration onto the shared parseCommandArgv codec and helper dedup. Verified:

  • npm run typecheck passes clean → no broken imports, lost exports, or .d.ts-rename fallout.
  • All CLI-related tests pass (2043); full suite green.
  • The collectivushypaware type-file rename is complete (only 3 intentionally-untouched links inside tombstoned LLPs remain).
  • Consolidated helpers originOf (ex-safeOrigin), isPlainObject (ex-isObject), and copyDir are byte-identical to what they replaced; the moved command bodies are verbatim.

The migration is faithful on the parts that matter: help-branch ordering ('help' in parsed before !parsed.ok), hyphenated-key reads (no typos), short aliases (-y/-f) wired, and every mutual-exclusion/cross-check rule preserved.

The findings below are all low-severity edge behaviors — none is caught failing by the suite, none is a common input.

Findings

1. readAllStdin now decodes per-chunk, corrupting multibyte UTF-8 split across chunk boundariessrc/core/cli/stdio.js:17
The old remote_commands.js copy accumulated Buffers then Buffer.concat(...).toString('utf8'); the new shared helper does out += chunk.toString('utf8') per chunk. A multibyte codepoint (emoji/CJK/accented) straddling a ~64 KB stream chunk boundary decodes to U+FFFD. Live callers (hyp remote login static token, enrollCentralSink join token) .trim() an ASCII token, so real impact is ~nil — but it's a silent regression in a generic helper. Cheap fix: keep the buffer-concat form.

2. hyp init --retention-days changed from parseInt to strict integer coercionsrc/core/commands/init.js:402
Old init used Number.parseInt(value, 10); the schema now uses { type: 'integer' }. Divergences: --retention-days 30.5 was accepted as 30, now errors (exit 2); --retention-days 1e3 was 1, now silently 1000. Note also that backfill.js kept type: 'number' for the same flag, so init and backfill now coerce --retention-days differently. Stricter is arguably better, but it's a behavior change on a script-facing flag worth a conscious call.

3. -h/--help anywhere in argv is reinterpreted as help, across all migrated commandssrc/core/cli/verb_codec.js:222
parseCommandArgv scans the whole argv for -h/--help before parsing, so a flag value equal to those strings can no longer be passed literally: hyp join <url> --token-file -h, hyp daemon install --config -h, hyp config validate --path -h now print usage instead of using -h as the value (old parsers took it literally). Extremely unlikely inputs, but a genuine cross-cutting change in the shared wrapper.

4. Greedy array positional doesn't trim or skip empty tokens, unlike the flag formsrc/core/cli/verb_codec.js:409
The new greedy-array branch in bindPositionals collects each remaining token raw, while the comma/flag array path (coerceValue, line ~367) does p.trim() + if (!p) continue. So hyp backfill claude "" codex yields ['claude','','codex'] (an empty provider name flows downstream), whereas --providers=claude,,codex yields ['claude','codex']. Same logical input, different result depending on form.

5. parseMcpArgv lost its exportsrc/core/commands/mcp.js:93
It was exported from core_commands.js in master, now module-private. No in-tree importer, so nothing breaks — flagging only because it's the one old export with no re-export replacement (out-of-tree consumers would break).

Minor / non-blocking notes

  • Several rejection messages changed wording without changing exit codes (backfill plan --since lost its targeted "not valid on plan" message → generic "unknown flag"; hyp leave foo, hyp mcp --remote --http, etc.). Cosmetic unless something pins stderr text — no in-repo test does.
  • Pre-existing (not caused by this PR): the MCP path validateToolArguments still doesn't enforce items.enum on array args, so the enum-propagation added to the CLI paths here widens a CLI/MCP asymmetry. Latent — no current MCP-exposed schema uses items.enum.

None of these block merge. #1 is the only one I'd actually fix before merging (one-line revert to buffer-concat); the rest are judgment calls you may prefer as-is.

The deduped readAllStdin decoded each stream chunk independently, so a
multibyte UTF-8 codepoint split across a chunk boundary became U+FFFD
replacement chars. Collect raw buffers and decode once, restoring the
pre-dedup behavior.
@platypii

Copy link
Copy Markdown
ContributorAuthor

Addressed top issue, merging.

@platypii
platypii merged commit e902f6e into masterJul 5, 2026
4 checks passed
@platypii
platypii deleted the cli-cleanup branch July 5, 2026 19:12
philcunliffe added a commit that referenced this pull request Jul 30, 2026
Type-only trim. Each removed declaration had exactly one occurrence in
the tree (its own definition): no importer, no `@import`, no test
reference, no `@ref` annotation, and no re-export from any public entry.
- `src/core/runtime/types.d.ts`: `CreateKernelRuntimeArgs`,
`CreateActivationContextArgs`, `CreatePluginPathsArgs`,
`ActivatePluginsArgs`, `ActivatePluginsResult`. Leftovers from the
paired-.d.ts consolidation (#182/#183); `activation.js` and `paths.js`
spell these param shapes inline in JSDoc instead.
- `src/core/types.d.ts`: `ResolveDependenciesOptions`,
`ManifestLoadResult`. `manifest.js` writes
`Promise<LoadedManifest|FailedManifest>` directly; both members stay.
- `src/core/cli/types.d.ts`: `WalkthroughResult`, orphaned by "delete the
dead walkthrough" (#241).
Removing those left `PluginPaths` and `SourceWithholdResolver` as unused
type imports in `src/core/runtime/types.d.ts`; dropped both.
No runtime code changed. npm test: 3005 pass / 8 fail / 1 skipped, the
same eight `leave-command.test.js` failures present on origin/master.
npm run typecheck clean. `npm run smoke -- core_boot_noop` ok.
Co-authored-by: test <test@test.com>
Co-authored-by: Claude <noreply@anthropic.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant

@platypii