Skip to content

fix(pull): give every machine the same copy of a same-name skill, and take deleted copies off the record - #1584

Merged
SaulMoro merged 12 commits into
Tencent:mainfrom
hz89Dev:fix/pull-shared-skill-wins-1577
Oct 30, 2026
Merged

SaulMoro merged 12 commits into
Tencent:mainfrom
hz89Dev:fix/pull-shared-skill-wins-1577

Conversation

@hz89Dev

@hz89Dev hz89Dev commented Oct 30, 2026 •

Copy link
Copy Markdown
Contributor

Summary

A member with no role gets every skill under skills/, namespaces included. When skills/release and skills/dev/release share a name, which copy they got depended on the order the filesystem listed directories, so two machines on the same team could run different skills. Now the shared copy always wins, pull names each copy it skipped and how to get it, and the dashboard shows the same.

skills/release/SKILL.md          shared
skills/dev/release/SKILL.md      role backend receives dev
skills/zzz/release/SKILL.md      no role receives zzz

no role       → skills/release, on every machine
role backend  → skills/dev/release (#707 unchanged)

The second part makes deleting a delivered copy one operation. removeDelivered(ledger, dest) deletes the copy and its entry in the checkout's delivery record, so the record never names a file that is gone. Every pull-side sweep, the already-synced cleanups and teamai remove go through it, and a repository lint rule, teamai/remove-delivered, fails npm run lint on a bare remove( in pull's files.

 src/pull.ts, src/resources/{skills,rules,agents}.ts
-  await remove(dest);
-  if (ledger) forgetDelivered(ledger.hashes, dest);
+  await removeDelivered(ledger, dest);
 oxlint-plugin-teamai/
+├── index.js                       # registers teamai/<rule>
+├── rules/remove-delivered.js      # + .test.ts, run through real oxlint
+└── test/harness.ts                # ruleHarness(rule).valid / .invalid

Dashboard, Resources → Skills: the skipped copy says which copy is delivered instead and which roles would deliver it. Below 1100 px the table becomes cards.

Desktop 1280 Tablet 768 Phone 375 Phone 375, zh-CN
desktop tablet phone phone zh-CN

Type of Change

  • Bug fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • Breaking change (fix or feature causing existing behavior to change)
  • Documentation only
  • Refactor / internal cleanup

Test Plan

Tested commit 10735bb5 on macOS, Node v24.21.0 (lint also on v20.19.5), 2026-10-30.

  • npm run typecheck (now also checks oxlint-plugin-teamai/) passes
  • npm run lint passes on Node 24 and Node 20.19.5
  • npx vitest run passes except gitlab-mr-timeout, which fails the same way on main
  • Added/updated tests for the change

Red on main, green here:

desired-skills › without a role, with directories listed descending
  › delivers the shared skill over a namespace skill of the same name

New tests:

  • oxlint-plugin-teamai/rules/remove-delivered.test.ts: 7 cases through real oxlint (each pull file reported with a message naming removeDelivered, removeDelivered allowed, other files and a reasoned disable allowed).
  • desired-skills / workspace-children / dashboard-resources-skills: both listing orders, namespace vs namespace, the roles named, exclusions, a workspace that holds the name back.
  • e2e shared-skill-wins-1577 (2) and remove-delivered-record-1577 (7): pull exclusion sweep, already-synced tombstone cleanup, uninstall --agent claude with CodeBuddy kept, full uninstall, teamai remove for skills, rules and agents. The existing record tests pass unchanged.
  • push-active-namespaces › "resolves a legacy delivered duplicate…" built its legacy copy by letting a pull with no role deliver z-archive's copy, which held only while the filesystem listed z-archive first. It now writes that copy and its record entry, as an older CLI could leave them, and still checks that push sends the edit to skills/z-archive/a-skill (100 passed, at 5fc5a14f; test-only change after 10735bb5).
  • Related e2e: git-exclude-uninstall-915, managed-resources-uninstall, remove-force, user-uninstall-keeps-projects-1025, namespaced-entries: 31 passed.

Real CLI verification

npm run build, then a throwaway sandbox (provider git, agent Claude, project scope) against dist/index.js at 10735bb5:

$ teamai pull --force   # member with no role
⚠ [workspace] Skipped skills/dev/release: the shared skills/release has the same name. To get it instead, set a role that receives dev: teamai roles set backend.
⚠ [workspace] Skipped skills/zzz/release: the shared skills/release has the same name, and no role receives zzz. Rename one of them in the team repo to deliver both.
✔ [workspace] Synced 1 skills (1 new, 0 updated)
✔ [workspace] Synced 1 rule(s)
delivered: description: "copy release"
$ teamai pull --force   # member with role backend
✔ [workspace] Synced 1 skills (1 new, 0 updated)
✔ [workspace] Synced 1 rule(s)
delivered: description: "copy dev/release"
record names commit-style.md: true
$ teamai remove rules commit-style --force
copy on disk: no; record names commit-style.md: false

Before, on main with the same release / zzz/release layout, the member with no role got skills/zzz/release on this machine.

Related Issues

Closes #1577

Notes for Reviewers

  • Uninstall: uninstall --agent <tool> takes that tool's deleted copies off every checkout's record, in either scope, and keeps entries under a root another installed tool shares. A full uninstall deletes the record together with .teamai, so it has nothing to edit.
  • Lint rule: the rules are JavaScript with JSDoc types because oxlint imports a plugin with plain Node, and CI lints on Node 20, which cannot strip TypeScript. npm run typecheck checks them with checkJs. The rule covers the files in PULL_SIDE_FILES; a new file that deletes delivered copies has to be added there (the plugin README says so).
  • Upgrade: a checkout already at the team's latest commit takes the "Already synced" path and keeps the copy it has until the next team commit; teamai pull --force switches it now. The distribution guide says so. Every selection change the CLI ships behaves this way.
  • Not covered locally: providers gitlab/github and agents Codex, CodeBuddy and OpenCode for the skill part; left to CI.

…without a role (Tencent#1577)

Without a role or project, pull listed skills/<name> and skills/<ns>/<name>
in readdir order and kept the last of each name, so two machines on one
revision could get different copies. Keep the shared copy, then namespaces
in name order, and say which copy was skipped and how to get it.
…o get the skipped one (Tencent#1577)

The Resources page labelled a namespace skill skipped for the shared copy
'Replaced by an active namespace skill', which is the opposite of what
happened for a member with no role. Name the copy delivered instead, the
roles that would deliver the skipped one (or that none does), and show the
skills table as cards below 1100px like the hand-off and settings tables,
so the delivery column is readable on a tablet or phone.

pull reads the same roles from the resolver, so its line names the role
to set instead of a placeholder.
…and lint for it (Tencent#1577)

Pull deleted a copy it delivered and took it off the delivery record in
two calls written at each site. Three sweeps (top-level skills, inactive
role agents, replaced root rules) had only the delete, and so did the
Codex dedupe, the inactive-namespace skill sweep and the leftover-file
sweep, each leaving a stale entry behind. teamai remove deleted copies
without saving the record at all, and uninstall --agent kept the
entries of the copies it deleted.

removeDelivered(ledger, dest) does both. Every pull-side removal goes
through it, teamai remove saves the checkout record after its
deletions, and uninstall --agent drops the entries of that tool's
deleted copies. A full uninstall takes the record away with the
checkout's teamai data.

oxlint-plugin-teamai holds the repository's own lint rules, one file
and one real-oxlint test per rule. teamai/remove-delivered fails a bare
remove( in pull.ts and the skills, rules and agents handlers; a path
pull never records carries a disable with its reason.
…20 (Tencent#1577)

oxlint imports a jsPlugin with plain Node, which strips TypeScript only from
22.18, and CI lints on Node 20. The rules stay type-checked through JSDoc and
checkJs; the lint floor is back to ^20.19 or >=22.12.
…Tencent#1577)

Reclaiming a reserved rule copy and the tombstone cleanup on the fast path
took entries off a ledger nothing saved, so the record kept naming the
deleted copy. Both now edit the checkout's record, which is written only
when it changed.
…ncent#1577)

When the workspace's members disagree on a same-name skill, no copy is
delivered, so no copy wins. The skipped line now also names the copy the
workspace actually delivers.
…ent#1577)

A role that receives two namespaces holding the same skill name stops at a
delivery conflict, so it delivers neither copy.
@github-actions

Copy link
Copy Markdown

No confident findings in the specified PR diff.

The PR description satisfies the testing requirement: it records a build followed by representative real-CLI verification at 10735bb5. The deferred provider/agent coverage is not blocking.

Review was read-only; I did not run, build, or install PR code.

@SaulMoro SaulMoro left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was generated by AI during triage.

Reviewed at 10735bb against #1577, both parts. I ran the built CLI at this head and at main (cfd798e) on the same fixture (git provider, Claude, project scope):

Case main this head
No role, pull --force delivers skills/zzz/release, no message delivers the shared skills/release, one Skipped line per copy
Following the advice (teamai roles set backend, pull) delivery record keeps an entry for a deleted copy delivers skills/dev/release, record clean
Team with no roles.yaml delivers skills/zzz/release the line says to rename and does not suggest teamai roles set (which fails there)
teamai remove of a skill and a rule record keeps both deleted files both entries dropped
Machine that had the namespace copy — "Already synced" keeps it until the next team commit, as the docs say; then pull switches it and the record matches

Criteria 1-6 and the definition of done are met: every pull-side removal goes through removeDelivered, teamai/remove-delivered fails npm run lint with a message naming it, and the three record tests are unchanged. The CI review reported no findings; I agree.

The workflow and package changes are needed and safe: ci.yml runs npm run typecheck (the old tsc --noEmit plus the plugin), npm run lint is the same command, and @oxlint/plugins is a pinned dev dependency that is not shipped. Lint with the plugin passes on Node 20, 22 and 24. The Windows job runs no lint, so the plugin is not exercised there.

Non-blocking:

  • [P3 nit] The rule covers the four listed files and only a call written remove(; fse.remove, fs.rm or a delete in a new pull-side file still passes.
  • [P3 nit] oxlint-plugin-teamai/README.md says helpers go in utils.ts; the file is utils.js.
  • [P3 nit] "To get it instead, set a role" does not say that a role also stops the shared root skills from being delivered by default.

Approving; this merges once CI is green at this head.

@SaulMoro

Copy link
Copy Markdown
Collaborator

This was generated by AI during triage.

E2E shard 1/3 fails on a test this PR's behavior change reaches: push-active-namespaces.test.ts:367 › "resolves a legacy delivered duplicate against every namespace, not only the first". The fixture has skills/svc-a/a-skill and skills/z-archive/a-skill, no shared copy, and a member with no role. The test expects the legacy pull to deliver the z-archive copy (the last one listed); at this head the pull delivers svc-a. That copy is now chosen deterministically, but the test exists to cover push resolving a delivered duplicate from a later namespace, so it needs that copy on disk. Seeding .claude/skills/a-skill with the z-archive content and recording it as delivered (as the test at line 337 does with recordDeliveredCopies), instead of getting it from pull, keeps what the test covers. If namespace-vs-namespace order was meant to stay as before, the docs should say which copy wins instead.

This holds the merge until the shard is green.

…cent#1577)

The test let a pull with no role deliver z-archive's copy, which held only
while the filesystem listed z-archive first. Pull now delivers the same copy
on every machine, so the test writes the copy an older CLI could leave, with
its record entry, and still checks that push sends the edit back there.
@github-actions

Copy link
Copy Markdown
  • [P2 non-blocking] Compare namespace names, not full paths — src/resources/desired.ts:266: With skills/dev/release and skills/dev-prod/release but no shared copy, the comparator selects dev-prod because - sorts before /. This contradicts the documented namespace-name ordering, where dev comes first. Compare namespace before using relativePath as a tie-breaker.

The PR description satisfies the testing requirement: it records a build and representative real-CLI verification at 10735bb5. The older tested commit and coverage deferred to CI are not blocking.

Review was read-only; I did not run, build, or install PR code.

@SaulMoro SaulMoro left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was generated by AI during triage.

Re-approving at 5fc5a14. The only change is the test fix for the E2E failure: it now writes the legacy z-archive copy with its record entry instead of getting it from pull, so it still covers push resolving a duplicate delivered from a later namespace.

The CI review's [P2 non-blocking] is right: src/resources/desired.ts:266 compares relativePath, so with skills/dev/release and skills/dev-prod/release and no shared copy, dev-prod wins (- sorts before /), while the docs order by namespace name. Comparing namespace first and keeping relativePath as the tie-breaker fixes it. It only affects two namespaces whose names share a prefix, so it does not block.

Merges once CI is green at this head.

…t#1577)

Comparing full paths put skills/dev-prod/ before skills/dev/, since '-'
sorts before '/'. The guide says the namespace whose name sorts first wins.
@hz89Dev

hz89Dev commented Oct 30, 2026

Copy link
Copy Markdown
Contributor Author

Fixed in 48beca1: sharedFirst now compares the namespace name first and the path only after that, so skills/dev/release wins over skills/dev-prod/release. The new test desired-skills › … orders by namespace name, so dev wins over dev-prod covers both listing orders, and it failed before the change.

@SaulMoro SaulMoro left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was generated by AI during triage.

Re-approving at 48beca1: sharedFirst now orders by namespace name before the path, so skills/dev/release wins over skills/dev-prod/release, with a test for it. That closes the CI review's [P2 non-blocking]. Merges once CI is green at this head.

@github-actions

Copy link
Copy Markdown

No confident new findings in the specified PR diff.

The earlier namespace-ordering finding is resolved: the comparator now compares namespace names before relative paths.

The PR description satisfies the testing requirement: it records a build followed by representative real-CLI verification at 10735bb5. The older tested commit and provider/agent coverage deferred to CI are not blocking.

Review was read-only; I did not run, build, or install PR code.

@SaulMoro
SaulMoro merged commit 254fb92 into Tencent:main Oct 30, 2026
18 checks passed
SaulMoro added a commit to SaulMoro/teamai-cli that referenced this pull request Oct 30, 2026
Takes main's teamai- prefix for default workflows, so the benchmark
default is workflows/teamai-benchmark.yaml and a team override uses that
name. Main's removeDelivered (Tencent#1584) replaces this branch's
forgetDelivered calls in pull, agents and rules; the net pull diff
against main is now only the benchmarks resource and the suite cache.
The benchmark help and commands.md now name the workflow teamai-benchmark.

Checked: tsc, lint, build; unit shards 1/2 and 2/2 (only the GitLab
timeout tests fail, as they do on origin/main here); e2e benchmark-,
workflow-from and dashboard-workflow-runs (21 files, 159 tests);
workflow conformance as local (63), skill (53) and skill-bare (53).
Sign up for free to 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.

pull: deliver same-name skills in a fixed order, and remove delivered copies through one operation

3 participants