Skip to content

Add CI workflow + fix 7 correctness bugs - #11

Open
gfargo wants to merge 8 commits into
pea3nut:masterfrom
gfargo:master
Open

Add CI workflow + fix 7 correctness bugs#11
gfargo wants to merge 8 commits into
pea3nut:masterfrom
gfargo:master

Conversation

@gfargo

@gfargo gfargo commented Jul 10, 2026 •
edited
Loading

Copy link
Copy Markdown

Hi @pea3nut,

First off, thanks for building agent-add! I've been using it across a couple of projects and have really been loving it so far. Hoping you'd be up for some third party contributions to tighten up a few things as well as propose a few new ideas I had.

This bundles 8 small changes. Each one started as its own PR against my fork, with its own tests and CI, before I combined them here - linked below if you want to look at any individually. No new features, just CI and bug fixes. If you'd rather review these as separate PRs instead of one bundle, just say so and I'll split it back apart.

Here's what's in it:

  • CI workflow (#1): there wasn't any before, the test badge was being updated by hand. Added a lint, test, and build workflow across Node 20/22/24. Node 18 isn't in the test matrix since vitest 4 (via rolldown) needs Node 20+ to run, which is a separate question from whether the published CLI still works on Node 18 at runtime (more on that in the linked issues).
  • Temp dir cleanup (#2): cloned or downloaded temp dirs never got cleaned up when asset validation failed, since process.exit() skips pending finally blocks. Every failed install left junk behind in os.tmpdir().
  • Git ref/subPath parsing (#3): two separate bugs in the same ~15 lines. An @ in embedded credentials (https://token@github.com/...) got mistaken for the @ref separator and truncated the URL. Separately, normalizeGitUrl's own output format for GitHub /tree/<ref>/<path> URLs (repo.git#path@ref) wasn't something resolveGit could parse, since it only looked for @ref before the #, so the ref silently fell back to the default branch and the subpath pointed at something that doesn't exist.
  • Source type detection (#4): any http(s) URL with a # got assumed to be a git repo, so a plain file URL with a fragment (https://example.com/prompt.md#usage) would try to git clone instead of just fetching it.
  • Name inference (#5): stripped everything after the last ., fine for files but wrong for skill directories. A folder named pdf.js-tools was getting cut down to pdf.
  • TTY detection (#6): the interactive host prompt only checked stdout.isTTY, so piping stdin while stdout was still a real terminal would hang instead of failing cleanly.
  • Error messages (#7): a few error messages were in Chinese while the rest of the CLI is English, so I translated those. No behavior change.
  • MCP/skill handler bugs (#8, the biggest one): three bugs stacked together. Installing a wrapped {"mcpServers":{"name":{...}}} source for a TOML host (Codex/Vibe) wrote the entry under the wrong name, since the TOML path was handed a stale variable. Both TOML formats reported an existing entry as exists regardless of whether the content matched, so an updated config silently got dropped on reinstall. And an existing but malformed JSON config file (a stray trailing comma from a manual edit, say) got treated as empty and overwritten, wiping out everything else in it. That last one's a real data loss bug, worth prioritizing if you only look at one of these.

A handful of other things came up along the way that felt like they needed a decision from you rather than a guess from me, so I filed those as issues instead of bundling them in here: https://github.com/pea3nut/agent-add/issues

Let me know if you'd rather I restructure any of this.

Resolves #1
Resolves #2
Resolves #3
Resolves #4
Resolves #5
Resolves #6
Resolves #7
Resolves #8

gfargo added 8 commits July 10, 2026 11:21
@gfargo
Add GitHub Actions CI workflow
a643c13 
* Add GitHub Actions CI workflow

Runs lint, tests, and build on push/PR to master across Node 18/20/22.
The repo currently has no CI — the test-results badge is updated
manually, so regressions can land without any automated check.

* Drop Node 18 from CI matrix — dev toolchain requires Node >=20

PR CI was failing on the Node 18.x job: vitest 4's transitive dependency
rolldown imports `styleText` from node:util, which doesn't exist until
Node 20. npm's EBADENGINE warnings during install confirm several
devDependencies (commander@14, vite@8, vitest@4, mute-stream@3,
@oxc-project/runtime) now require Node >=20.

This isn't a flake — Node 18 genuinely cannot run `npm test`/`npm run
build` with the current toolchain. Matrix now covers 20.x/22.x/24.x,
which is what's actually being verified as working.

Separately (not addressed here): the published CLI's dist bundle also
pulls in @inquirer/select, whose dependency chain calls the same
Node-20+-only `styleText` API eagerly at module-load time — so running
the built CLI on Node 18 likely fails too, despite package.json's
"engines": ">=18". Worth a follow-up to either bump the documented
minimum or address the dependency.
@gfargo
Fix temp-dir leak when asset validation fails (#2)
a5236e3 
resolveGit/resolveHttpFile clone or download sources into os.tmpdir()
before validateAsset runs. When validation failed, runInstaller called
process.exit(2) directly — which does not run pending finally blocks —
so the cleanup loop at the end of the function never executed and the
temp dir was left on disk.

Track resolved temp dirs in a Set as sources are resolved and clean
them up explicitly before every process.exit(2) call, not just on the
success path.
@gfargo
Fix git source ref/subPath parsing (#3)
0f5db9f 
Extracts the ref/subPath splitting logic from resolveGit into a pure,
unit-testable parseGitSource function and fixes two bugs found in it:

1. The @ref separator was found via the first "@" after an optional
   leading "git@" prefix. For HTTPS URLs with embedded credentials
   (https://token@github.com/org/repo.git@v1.0.0), this matched the
   credentials separator instead of the ref separator, truncating the
   repo URL to "https://token". The search now starts at the path
   segment (after the authority) for non-SSH URLs, so any @ in
   userinfo is skipped.

2. normalizeGitUrl (used for GitHub/GitLab /tree/<ref>/<path> web URLs)
   emits sources in "repo.git#path@ref" order, but resolveGit only ever
   looked for @ref in the part *before* "#" — so the ref was silently
   dropped (falling back to HEAD) and the literal string "path@ref" was
   used as the subPath, which doesn't exist in the checked-out repo.
   parseGitSource now also recognizes a trailing @ref after the
   subPath.

README's documented "repo.git@ref#path" ordering continues to work as
before.
@gfargo
Fix source-type detection misclassifying non-git HTTP file URLs (#4)
bbb96aa 
detectSourceType treated any http(s) URL containing "#" as a git
repository, so a plain file URL with a fragment (e.g.
https://example.com/prompt.md#usage) would attempt a git clone against
a non-git HTTP endpoint instead of being fetched directly. This
double-purposes "#" both as a subPath marker for git sources and as a
literal URL fragment for direct file downloads.

Only treat "#" as a git subPath marker when the URL portion before it
doesn't already look like a direct file this tool knows how to fetch
(.json/.md) — the only asset types resolved via plain HTTP download.
@gfargo
Fix stripExtension mangling directory-based skill asset names (#5)
ccb1eba 
inferName() always stripped everything after the last "." when
deriving an asset name from a local basename or a git #path last
segment. This is correct for file-based assets (mcp/.json,
prompt|command|subAgent/.md), but skill sources always resolve to a
directory — a dotted directory name like "pdf.js-tools" was being
truncated to "pdf".

Added an isDirectorySource option to inferName, passed from
installer.ts based on the asset type being installed. The git-repo-name
branch (repo.git -> repo) is unaffected, since stripping the ".git"
URL suffix is always correct there regardless of asset type.
@gfargo
Fix TTY detection to require both stdin and stdout be interactive (#6)
0cfb214 
The check for whether to show the interactive host-selection prompt
only looked at process.stdout.isTTY. An interactive prompt (via
@inquirer/select) needs to both read from an interactive stdin and
render to an interactive stdout — checking only stdout misses the case
where stdin is piped/redirected but stdout is a terminal (e.g. `agent-add
--mcp ... < /dev/null`), which would proceed into the prompt and hang
waiting on input that will never arrive interactively.

Now requires both process.stdin.isTTY and process.stdout.isTTY before
attempting the interactive prompt.
@gfargo
Unify error messages to English (#7)
0649adc 
Several validation and parsing error messages (installer.ts's
validateAsset, source/inline.ts, source/infer-name.ts) were written in
Chinese while the rest of the CLI's user-facing output (help text,
CLI errors, host capability messages) is English. Translated for
consistency; no behavior change.
@gfargo
Fix TOML MCP name/conflict bugs, config data-loss, and skill exists c…
ff3228f 
…heck (#8)

Three related correctness bugs found while auditing the asset handlers:

1. src/assets/mcp.ts: when dispatching to the TOML config path (used by
   Codex/Vibe), the handler passed the original `job` object instead of
   the locally-reassigned `assetName`. mcpHandler.handle() unwraps the
   {"mcpServers":{"name":{...}}} convenience format and reassigns its
   local `assetName` variable, but handleTomlMcp re-derived assetName
   from `job.assetName`, which is the *pre-unwrap* name (typically
   derived from the source filename). Installing a wrapped-format MCP
   source for a TOML host silently wrote the entry under the wrong key.
   Existing scenario tests didn't catch this because their fixtures
   aren't in wrapped format, so the reassignment path is never
   exercised.

2. Both TOML branches (Vibe's [[mcp_servers]] array and the default
   [mcp_servers.<name>] table format) treated any existing same-name
   entry as `status: 'exists'` without comparing content, unlike the
   JSON write path. Re-running an install with updated MCP config for a
   TOML host silently dropped the update instead of reporting a
   conflict.

3. src/utils/fs.ts: readJSONOrNull() caught *all* errors — including
   JSON.parse failures on an existing file — and returned null the same
   as a missing file. src/assets/mcp.ts defaults a null result to `{}`,
   so an existing-but-malformed config file was silently treated as
   empty and then overwritten via atomicWriteJSON, destroying every
   other entry it contained. Now only ENOENT (file not found) returns
   null; other errors propagate, and mcpHandler turns that into an
   'error' result instead of writing over the file.

4. src/assets/skill.ts: the "already installed" check only tested
   whether the target directory exists, not whether SKILL.md is present
   inside it. A directory left behind by an interrupted install (e.g.
   process killed mid-copyDirRecursive) would be permanently reported as
   'exists' on every future run, with no way to repair it short of
   manually deleting the directory. Now checks for the entry file
   itself, so an incomplete install is retried.
@gfargo

gfargo commented Aug 5, 2026

Copy link
Copy Markdown
Author

friendly bump on this @pea3nut 👋

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment