feat(release): tell tenjin when the MCP Registry pin goes stale - #181
Conversation
A successful `changeset publish` now files one issue on BackTrackCo/tenjin naming the newly published tenjin-cli version and the files that must move with the pin. The registry rejects version ranges, so the pin is exact and lives in the other repo, where nothing notices it going stale until the next registry publish preflight weeks later. Closes #98 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
vraspar
left a comment
There was a problem hiding this comment.
Review: solid notify-only shape; four minors, none blocking — approve after a look at the silent-skip paths
Reviewed against the diff and live cross-repo checks, verified at fe7da98. Ran locally: pnpm lint, pnpm typecheck, pnpm vitest run src/notify-registry-pin.test.ts (7/7), bash scripts/pack-smoke.sh (PASS incl. the new mcpName assertion).
What's solid:
- Notify-only is the right scope for #98's option 1: no auto-PR, no auto-publish, and the issue body names all three files that move together with correct paths — every cross-repo reference verified live against
BackTrackCo/tenjinmain (server.jsonreally pinspackages[0]at an exact version;lib/mcp/metadata.tsreally holdsMCP_SERVER_INFO;docs/MCP-REGISTRY.mdexists). - The loose-search + exact-jq-predicate dedupe correctly survives a still-open previous-version issue, and that regression has a dedicated test.
- Secret handling is clean: step-scoped token, masked by Actions, never echoed, mktemp body file trapped away; workflow is dispatch-only on
main, so fork PRs can't reach the PAT. - The test harness drives the real script and scrubs ambient
GH_TOKEN/GITHUB_*before each scenario — it stays honest when run inside Actions.
Minor
-
CI The trigger wiring can fail back into the exact silence this PR fixes: the whole feature hangs off two changesets/action output names (
published,publishedPackages) with no validation anywhere. If an action upgrade renames or drops an output,'' == 'true'skips the step wordlessly and green — "a release nobody was told about" again. Cheap fix: fail loudly if the step ran butpublishedis unset (distinguishes "action didn't report" from "reported false"), or add actionlint to CI so expression/output drift gets caught.
tenjin-agent/.github/workflows/release.yml
Lines 174 to 184 in fe7da98
-
Testing The gh stub couples to the jq's source text, not its semantics: the stub decides exact-match mode via
value('--jq').includes('env.TITLE'). Mutation probe: typo the jq to.numer // empty(substring intact) and the suite stays 7/7 green while real gh returns empty for every lookup — dedupe silently disabled forever. Fix: have the stub apply the predicate minimally, or key the mode off something observable rather than the literal substring.
tenjin-agent/src/notify-registry-pin.test.ts
Lines 35 to 37 in fe7da98
-
Hygiene A failed notification is unrecoverable by any rerun, and RELEASING.md doesn't say how to recover: once publish ships X and the notify step fails, "re-run failed jobs" republishes nothing → gate false → no retry path; the missed issue must be filed by hand, but no doc says so. One sentence in RELEASING.md (the env-overridable script supports it directly): run
PUBLISHED_PACKAGES='[{"name":"tenjin-cli","version":"X"}]' GH_TOKEN=… bash scripts/notify-registry-pin.sh.
Lines 42 to 46 in fe7da98
-
Security Dedupe rides best-effort search recall:
gh issue list --searchis loose full-text over a default 30-result page, and GitHub search indexing lags fresh issues by seconds–minutes — any of those makesexistingempty and files a duplicate. Impact is noise, not compromise (the workflow concurrency group already serializes real races). Add--limit 200and consider searching just"tenjin-cli@$version in:title"so prose words don't dilute recall.
tenjin-agent/scripts/notify-registry-pin.sh
Lines 52 to 53 in fe7da98
Nits (5), none blocking
- Hygiene The bash-3.2 comment states the parse bug more broadly than it exists (
scripts/notify-registry-pin.sh:65-67): empirically only a line ending in)breaks$()+ heredoc parsing, not any bare)mid-line — and the current body has no such line. Reword or drop the version archaeology; keep the temp-file approach. - Hygiene Token rationale triplicated across
release.yml:177-182,notify-registry-pin.sh:35-43,RELEASING.md:69-73; keep the full story in RELEASING.md, one-line pointers elsewhere. - Testing Test comment narrates tenjin's internal file history ("moved out of lib/mcp/server.ts",
src/notify-registry-pin.test.ts:144-146) — rots on the next tenjin refactor; trim to where the symbol lives now. - Security
TARGET_REPO/PACKAGEenv overrides are unconstrained in CI (notify-registry-pin.sh:19-20); harmless today (PAT-scoped), but they're pure surface in production — pin them in the workflow env. - Testing Untested boundaries: empty-string
GH_TOKEN(what Actions actually produces for an unset secret — same code path as missing, but free to cover), mixed published array (tenjin-clialongside other packages),GITHUB_SERVER_URLoverride shaping the run link.
Verified, not issues
- Injection cleared end-to-end: version comes from npm-enforced semver, reaches
ghonly as argv /env.TITLE(never re-parsed as a filter); every backtick in the heredoc body escaped, no$(...);GITHUB_*link vars are runner-set constants on a dispatch-only workflow. - PAT blast radius contained: fine-grained Issues-only on one repo fails auth elsewhere; token absent from logs and from the test stub log.
- pack-smoke's
mcpNameread hits the true packed artifact (tarball installed by path into an empty consumer dir) and runs on every PR via ci.yml, not just releases. - Stub's multi-match behavior matches jq's
map(...)[0]ordering; ignoring--searchin the stub is a conservative superset since the exact-title predicate decides correctness. - Malformed
PUBLISHED_PACKAGESdies loudly underset -e(raw node trace, cosmetic only); empty-string token caught by[ -z "${GH_TOKEN:-}" ]. - Red-run-after-successful-publish is documented intent, and workflow_dispatch-only means no required-check/auto-merge machinery misreads it.
Closed as decisions
- Loud failure marking an otherwise-successful release red until the owner adds
RELEASE_CROSSREPO_TOKEN: stated intent in the PR body, and arguably the point. Owner setup step is documented in RELEASING.md. - Notify-only, no auto-PR/auto-publish: explicit non-goal of #98 option 1.
Verdict: comments-only — no majors; minors 1–2 are cheap hardening of exactly the silence this PR exists to end, worth taking before merge; 3–4 and the nits are follow-up-grade.
The step gated on `published == 'true'`, so a changesets/action upgrade that renamed or dropped either output would skip it wordlessly and green: a release nobody was told about, the bug this feature exists to fix. The gate now runs the step for anything other than a literal false, and the script fails on an empty `published` or a claimed publish that names no package. Dedupe asks for a 200-issue page and searches the version phrase alone, so prose words cannot dilute recall, and RELEASING.md carries the manual recovery command for a notification that failed after npm already had the package. The gh stub now evaluates the script's --jq (predicate field, env var, emitted field) instead of matching its source text, so a typo like `.numer // empty` reds instead of passing while real gh returns empty for every lookup.
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
|
All four minors and all five nits are in Finding 1, the runtime assertion rather than actionlint. actionlint cannot catch this class: renaming Inverting that gate is only safe because of what the pinned action actually does. actionlint was run against the workflows and is clean, but not added to CI: it does not catch this bug, and a workflow-lint job is separate scope. Finding 2, the stub now applies the predicate. It parses the
Both probes reverted. Findings 3 and 4 landed as written: Your five untested boundaries are covered, including the mixed published array with Gates: format, lint, typecheck, build clean; 2153 tests pass; pack-smoke PASS. |
vraspar
left a comment
There was a problem hiding this comment.
Re-review: all four minors and five nits resolved at c987d75 — approving
Delta reviewed since fe7da98, both mutation probes independently re-run, gates verified locally (pnpm lint, pnpm typecheck, 13/13 tests, bash scripts/pack-smoke.sh PASS).
Resolved since round 1
- Silent-skip trigger wiring → resolved with the runtime assertion rather than actionlint. Verified the load-bearing claim against installed source: changesets/action at the pinned SHA
63a615bsetspublished=false/publishedPackages=[]unconditionally before any branch (src/index.ts:63-64), so the inverted gate!= 'false'cannot turn a genuine no-op dispatch red, while an output rename now lands as an emptyPUBLISHEDand fails loudly (release.yml:174). The disagreement check (published=true naming zero packages) closes the second silent path (notify-registry-pin.sh:44-49). - Circular stub coupling → resolved. The stub now parses and evaluates the jq program's shape and exits 1 on anything unsupported. Both probes re-run independently:
.number→.numertypo ⇒ 1 failed (dedupe case reds, exactly as real gh would behave); predicate removed entirely ⇒ 7 failed loudly instead of passing green. - Unrecoverable failed notification → resolved: RELEASING.md documents that a re-run cannot fix it and gives the by-hand command, including the now-required
PUBLISHED=true. - Best-effort dedupe recall → resolved:
--limit 200, query narrowed to"tenjin-cli@<version>" in:title, miss documented as noise-not-harm, lookup shape asserted by test.
Nits also landed: bash-3.2 archaeology replaced with a neutral rationale, token rationale deduplicated to pointers into RELEASING.md, file-history narration trimmed from the test comment, TARGET_REPO/PACKAGE pinned in workflow env, and the three boundary gaps covered (empty-string token as Actions actually delivers an unset secret, mixed published array ordered to catch prefix matches, non-github.com server URL).
Still open: none.
New: nothing beyond the above; the delta introduces no new issues.
Closed as decisions
- Dedupe remains best-effort (recall miss files a duplicate issue): stated plainly in code and accepted as noise, not harm.
- Loud red run after a successful publish until
RELEASE_CROSSREPO_TOKENexists: round-1 decision, unchanged. - Notify-only scope, no auto-PR/auto-publish: unchanged non-goal.
Verdict: approve. Clean at c987d75; the only owner action left is creating the RELEASE_CROSSREPO_TOKEN secret before the next release.
Scope
A successful
changeset publishnow files one issue onBackTrackCo/tenjinasking for the MCP Registry pin bump, so a release stops staling that listing in silence. Option 1 (notify) from #98; the auto-PR shape and auto-publishing stay out.scripts/notify-registry-pin.shreads the published version fromchangesets/action'spublishedPackages, dedupes on an exact open-issue title match, and files an issue namingserver.json,package.json, andlib/mcp/metadata.ts(whereMCP_SERVER_INFOlives since tenjin#493), plus the rule that the registry publish stays manual and follows a promoted deploy..github/workflows/release.ymlgates the new step onsteps.publish.outputs.published == 'true', so a dispatch that shipped nothing announces nothing.scripts/pack-smoke.shasserts the packedmcpNameisblog.tenjin/tenjin, the npm precondition Wire the MCP Registry packages pin into the release pipeline #98 asks for: dropping it breaks tenjin's next registry publish, not anything here.Risk: the step runs after npm already has the package, so a failure there cannot un-publish. Until the owner action below is done it will fail the release run (loudly, on purpose: an unnoticed miss is the bug being fixed).
Owner action required
Create repo secret
RELEASE_CROSSREPO_TOKEN: a fine-grained PAT scoped toBackTrackCo/tenjinwith Issues: read and write.#98 assumed the release-bot token already in that job would do. It will not: the
x402r-release-botinstallation holdscontents: write,pull_requests: write,metadata: readand no Issues permission, and it is installed on this repo (andx402r-sdk), not ontenjin. Widening the App would need both a new permission grant and a new repo in its installation, so the token is parameterized on a named secret instead.RELEASING.mdcarries this as one-time owner setup step 4.Testing
src/notify-registry-pin.test.ts: 7 cases driving the real script withghstubbed. Files once per version; files nothing on a no-op publish or when the publish carried notenjin-cli; skips a same-title open issue; still files when only the previous version's issue is open (the loose--searchalone would swallow it); fails and writes nothing without a token; links the release run.pnpm lint,pnpm typecheck,pnpm format:check: clean.pnpm test: 2147 passed, 10 skipped.bash scripts/pack-smoke.sh: PASS, including the newmcpNameassertion.gh's gojq resolvesenv.TITLE, which the dedupe query depends on.Closes #98