Fix dashboard contract follow-ups - #14
Conversation
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d2fb612fef
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| function compareVersionStrings(localVersion: string, remoteVersion: string): number { | ||
| const parse = (version: string): number[] | null => { | ||
| const match = version.trim().match(/^v?(\d+)\.(\d+)\.(\d+)(?:[-+].*)?$/); |
There was a problem hiding this comment.
For registry-installed skills whose versions include prerelease tags, this parser strips the suffix and compares only major/minor/patch. As a result, upgrading from 1.0.0-alpha to 1.0.0 is reported as up_to_date, so askill check --json and askill update --json skip the stable registry release even though SemVer orders prereleases below the corresponding normal version. Preserve and compare prerelease identifiers, or delegate to a SemVer comparator, before using this status to decide whether to update.
Useful? React with 👍 / 👎.
| if (entry.sourceType === 'registry') { | ||
| results.push(await checkRegistrySkillVersion(name, entry, scope, base)); | ||
| continue; |
There was a problem hiding this comment.
When a registry-installed skill has an older local version, this new registry check only feeds buildCheckPayload, so it is used by the --json paths only. The normal askill check / askill update -y loops below still gate on GitHub sources and therefore report registry skills as uncheckable or say nothing is available to update. Share this registry version check with the non-JSON update flow so CLI users get the same update behavior as dashboard integrations.
Useful? React with 👍 / 👎.
| const parsed = parseSource(source); | ||
|
|
||
| const applyExplicitSkillFilter = (skills: DiscoveredSkill[]): DiscoveredSkill[] => { | ||
| return options.skill ? filterSkills(skills, [options.skill]) : skills; |
There was a problem hiding this comment.
--skill to registry refs inside collections
When --skill is used with a shared collection whose entries resolve to registry slugs, the recursive resolveSkills() call reaches the registry branch, but this new filter is only applied in the local/Git paths. That means a non-interactive command such as askill add col:owner/tools --skill formatter -y --json still installs every registry-backed collection entry instead of the requested skill. Apply the explicit filter after registry resolution, or filter the collection's resolved skills before returning.
Useful? React with 👍 / 👎.
|
Pushed follow-up commit What changed:
Validation:
This addresses the three Codex P2 review comments and solidifies the new PR13/PR14 behavior in E2E. |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 389d788bc2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
|
|
||
| async function getInstalledSkillVersion(skillName: string, globalScope: boolean): Promise<string | null> { | ||
| const installedSkills = await listInstalledSkills({ global: globalScope }); |
There was a problem hiding this comment.
When a registry skill was installed with --copy, installSkill writes only to the agent-specific directory and does not create the canonical .agents/skills/<skill> entry that listInstalledSkills() enumerates. In that scenario this returns no installed skill, so checkRegistrySkillVersion() reports no local version recorded and both askill check and askill update can never update a valid copy-mode registry install. The version lookup needs to fall back to the installed agent paths or the lock's selected agents instead of only canonical skills.
Useful? React with 👍 / 👎.
| const parsed = parseSource(source); | ||
|
|
||
| const applyExplicitSkillFilter = (skills: DiscoveredSkill[]): DiscoveredSkill[] => { | ||
| return options.skill ? filterSkills(skills, [options.skill]) : skills; |
There was a problem hiding this comment.
If the user passes a typo such as askill add ./skills --skill formater -y --json, this filter silently returns an empty list and the later install path treats it like a successful no-op (ok: true with no skills) rather than an invalid selection. For CI/dashboard callers using the new non-interactive selector, that makes a requested install appear successful even though no skill was installed; the explicit --skill path should produce a non-zero/error when the source had skills but none matched.
Useful? React with 👍 / 👎.
| options.all = true; | ||
| } else if (arg === '--json') { | ||
| options.json = true; | ||
| } else if (arg === '--skill') { |
There was a problem hiding this comment.
When --skill is present without a following value, for example askill add ./skills --skill -y --json, this branch leaves options.skill unset and the non-interactive install path proceeds as if no selector was requested, installing every discovered skill. Since this option is meant to constrain multi-skill installs, the parser should reject the missing value instead of silently broadening the install.
Useful? React with 👍 / 👎.
| } | ||
|
|
||
| function latestRegistrySlug(source: string): string { | ||
| const match = source.match(/^(@[^/]+\/[^@/]+)(?:@[^/]+)?$/); |
There was a problem hiding this comment.
For a registry install whose lock source includes a pinned version or range, such as @anthropic/memory@^1.0.0, this strips the constraint before checking and updating. If the registry's latest release is 2.0.0, askill check will report an update and askill update will install it even though the original source explicitly constrained the allowed version; preserve the version/range when querying or filter the latest result with semver.
Useful? React with 👍 / 👎.
| const installErrors: string[] = []; | ||
|
|
||
| for (const agent of targetAgents) { | ||
| const result = await installSkill(resolvedName, content, agent, { mode: 'symlink', global: lockOptions.global }); |
There was a problem hiding this comment.
If a registry skill's remote SKILL.md changes its frontmatter name, this installs the update into a new directory while the lock entry is still updated under check.name and the old skill directory is left in place. Subsequent checks continue reading the old installation and report the same update again, so updates never converge for renamed registry skills; either install in place using the locked name or migrate/remove the old lock and directory together.
Useful? React with 👍 / 👎.
|
Pushed follow-up commit Fixes:
New E2E coverage:
Validation:
|
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f98d7aa23b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| async function getRegistrySkillVersion(source: string): Promise<string | null> { | ||
| const content = await api.getSkillRaw(latestRegistrySlug(source)); | ||
| return stringOrNull(parseSkillMd(content).frontmatter.version); |
There was a problem hiding this comment.
When a locked registry source includes a semver range such as @scope/skill@^1.0.0, this helper strips the range before calling the registry, so askill check queries the latest overall version rather than the latest version satisfying the locked range. If 1.1.0 exists and 2.0.0 is the overall latest, the later semver.satisfies check marks the skill up_to_date because 2.0.0 is outside the range, causing users to miss the valid 1.x update; the update path also uses the same stripped slug. Query the range-aware source (or otherwise resolve the newest matching version) instead of dropping the range.
Useful? React with 👍 / 👎.
| for (const agent of targetAgents) { | ||
| const result = await installSkill(check.name, content, agent, { mode: 'symlink', global: lockOptions.global }); |
There was a problem hiding this comment.
For registry skills originally installed with --copy, askill update now always calls installSkill with mode: 'symlink', which replaces the copied agent directory with a symlink and creates the canonical .agents/skills copy. This changes the install layout for users who explicitly chose copy mode (for example to avoid symlinks or keep per-agent copies) as soon as a registry update is applied; detect the existing layout or recorded install mode before forcing symlink mode.
Useful? React with 👍 / 👎.
| if (collectionItems.length > 0 && resolvedSkills.length === 0) { | ||
| throw new Error(`Collection ${collectionOwner}/${collectionHandle} does not contain any installable skills`); |
There was a problem hiding this comment.
When a collection item lacks skillName, using --skill with a non-existent name still includes that item in collectionItems; after each candidate resolves to an empty filtered result, this throw happens before runInstallJson can emit the intended SKILL_NOT_FOUND response. In that collection metadata shape, askill add col:... --skill missing --json returns UNHANDLED_ERROR instead of the structured selector error, so dashboards cannot reliably handle no-match cases.
Useful? React with 👍 / 👎.
| if (options.skill) { | ||
| printJsonError('SKILL_NOT_FOUND', `Skill "${options.skill}" not found in source`, { |
There was a problem hiding this comment.
When --skill filters a cloned Git/GitHub source down to zero matches, tempDir has already been created, but printJsonError calls process.exit(1) from inside the try block so the finally cleanup below is bypassed. Repeated JSON-mode selector misses against remote repos will leave cloned temp directories behind; emit the error in a way that lets the existing cleanup run first.
Useful? React with 👍 / 👎.
|
Updated PR #14 with latest Codex review follow-ups. Changes:
Validation:
|
|
@codex review |
|
Codex Review: Didn't find any major issues. Hooray! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
1 participant
Summary
askill add --skill <name>for non-interactive local multi-skill installs..agents/skills/<name>re-linking idempotent while preserving existing lock source metadata.Tests
npm run buildnpm testnode -e \"JSON.parse(require('fs').readFileSync('docs/json-contracts/askill-cli-json.schema.json','utf8')); console.log('schema ok')\"npm run test:e2e:builddocker run --rm askill-test test/e2e/run.sh test_dashboard_json_contracts test_add_skill_selector_local test_add_canonical_path_relinks_agentnpm run test:e2e(207 passed, 0 failed, 1 skipped existing production collection fixture)Follow-up for #11.