Fix issues found by an Opus review pass - #12
Merged
Conversation
… and the skill Critical: bump-homebrew.yml and clawhub-publish.yml never actually ran on a real release. Both trigger on release: published, but release.yml creates that release with the default GITHUB_TOKEN, and GitHub does not let events from that token trigger other workflows. Both are now called directly as jobs (workflow_call) right after npm publish, instead of relying on the release event. Other fixes: - bump-homebrew.yml: curl now fails loudly on a bad download instead of hashing an error body as a fake sha256; version is validated as bare semver and passed through env: instead of interpolated into the shell; the formula sed rewrite is now asserted to have actually taken. - src/commands.ts: CLI_VERSION was a hardcoded literal that was already stale one release behind, mislabeling agent-context output and every pairing token. Now read from package.json like --version already is. - src/argv.ts: --timeout was missing from the recognized global flags, so it errored as an unknown command when placed before the subcommand, unlike --base-url and --token. - README.md: still said "Not published yet" on the actual npm package page for a package that's been live for two releases. - skills/charming/templates/crud moved from the repo root into the skill folder — ClawHub and Hermes only sync skills/<name>/, so the template the skill tells agents to always copy was invisible to both. - templates/crud/ui.js: the 4s poll did a full #app innerHTML replace, wiping half-typed input and cutting the undo toast's display window short every time it fired. #toast and the list are now separate, independently-updated containers, and in-progress input in #add-input is preserved across a background refresh. item.id/item.text are now HTML-escaped before interpolation (was unescaped, stored XSS in every app copied from the template). Verified: bun run check passes clean (112 tests). Rebuilt, packed, and globally installed the tarball fresh — confirmed --version and agent-context both report 0.1.1, --timeout works before the subcommand, and the template ships under skills/charming/templates/crud. Re-ran a clawhub sync dry-run against the moved skill folder — still resolves correctly (6 files, status: update). Not fixed (cosmetic, deferred): stale @usecharming/cli name in bun.lock, workflow step ordering in release.yml, no concurrency group on the release workflows.
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
1 participant
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 Opus review agents went over the workflows, CLI source, and the ClawHub/Hermes skill built this session. Real findings, fixed here:
Critical
bump-homebrew.ymlandclawhub-publish.ymltriggered onrelease: published, butrelease.ymlcreates that release with the defaultGITHUB_TOKEN— GitHub doesn't let events from that token trigger other workflows. Confirmed by run history: zerorelease-event runs ever existed for either workflow. Both are now called directly as jobs (workflow_call) right afternpm publish, so the publish → Homebrew/ClawHub chain actually fires.High
bump-homebrew.yml: a failedcurl(404, wrong version) silently hashed the error body as a fake sha256 and opened a plausible-looking bad PR. Now fails loudly and validates the tarball.templates/crudlived at the repo root, not insideskills/charming/— ClawHub and Hermes only syncskills/<name>/, so the template the skill tells agents to "always copy, never free-hand" was invisible to both. Moved in.#appreplace, wiping half-typed input and cutting the undo-toast window short every ~4s. Split into a persistent#toast+ a narrower re-rendered container, per the skill's own re-render rule.Medium
CLI_VERSIONwas a hardcoded literal, already stale one release behind — mislabeledagent-contextoutput and every pairing token. Now read frompackage.json.--timeoutwas missing fromglobalFlagsinargv.ts— errored as an unknown command when placed before the subcommand, unlike--base-url/--token. Added, with a regression test.README.mdsaid "Not published yet" on the live npm package page.item.id/item.textintoinnerHTMLunescaped — stored XSS in every app copied from it. Escaped.${{ inputs.version }}interpolated directly into shell inbump-homebrew.yml; moved throughenv:and validated as bare semver.sedrewrite could silently no-op if the formula's format ever shifted; now asserted.Verified
bun run check— clean, 112 tests.--versionandagent-contextboth report0.1.1,--timeoutworks before the subcommand, template ships underskills/charming/templates/crud.clawhub sync --dry-runagainst the moved skill folder — resolves correctly (6 files,status: update).Deferred (cosmetic, listed not fixed)
Stale
@usecharming/cliname inbun.lock,release.ymlstep ordering, noconcurrency:group on the release workflows.