Skip to content

fix(usage): scope skill-usage events by cwd so one project's usage never reaches another team's stats - #750

Closed
ydflow wants to merge 1 commit into
Tencent:mainfrom
ydflow:fix/usage-scope
Closed

ydflow wants to merge 1 commit into
Tencent:mainfrom
ydflow:fix/usage-scope

Conversation

@ydflow

@ydflow ydflow commented Sep 23, 2026

Copy link
Copy Markdown
Contributor

Summary

Skill-usage events are written by the same hooks as dashboard events, but only
dashboard events carry a cwd. reportUsageToTeam therefore filters one stream
by scope and aggregates the other whole, so a machine holding several projects
ships every project's usage to whatever repo pull happens to be reporting to.

-reportUsageToTeam(repoPath, user, opts)
-  events = readUsageEvents()                 // every project on the machine
+reportUsageToTeam(repoPath, user, opts)
+  events = filterEventsByScope(readUsageEvents(), opts)

This is bug 2 of #748. Bug 1 — the Stop-hook nudge reaching projects that never
initialized teamai — is left alone: #747 is already reworking
contributeHintAllowed, which is where that fix belongs.

What changed

UsageEvent gains an optional cwd, populated at every hook write site through
the existing resolveHookCwd helper (which already handles Cursor's
workspace_roots and treats an empty cwd as missing). filterEventsByScope
becomes generic over any event carrying a cwd, so both streams share one set of
path rules — including the Windows separator/case folding that the dashboard
tests already pin.

-export function filterEventsByScope(events: DashboardEvent[], ...): DashboardEvent[]
+export function filterEventsByScope<T extends { cwd?: string }>(events: T[], ...): T[]

Write sites:

Site cwd source
hook-handlers.ts track handler resolveHookCwd(stdin)
hook-handlers.ts track-slash handler resolveHookCwd(stdin)
usage-tracker.ts trackFromStdin resolveHookCwd(hookData)
usage-tracker.ts trackSlashCommand resolveHookCwd(hookData)

The legacy teamai track <toolName> <toolInput> form takes only a tool-input
string and has no hook payload to read a cwd from, so its events keep cwd
undefined. No production hook uses it — the dispatcher path is the one above.

Behavior for events without a cwd

Events written before this field, and events from a hook that ran without a
cwd, have no value. The two filters treat them asymmetrically, matching how
dashboard events are already treated:

  • a projectRoot report drops them — they cannot be shown to belong to that project;
  • an excludeProjectRoots report keeps them — they cannot be shown to belong to an excluded one.

pull reports to both scopes with complementary filters, so nothing is lost from
the local queue: the union of the two targets covers every event, and
truncateUsageAfterReport still counts the unfiltered file length.

Test Plan

Unit tests

npx vitest run for the touched suites:

src/__tests__/scope-filter.test.ts            21 passed
src/__tests__/team-push-interventions.test.ts 11 passed
src/__tests__/hook-handlers.test.ts
src/__tests__/pull-scope-isolation.test.ts
src/__tests__/stats.test.ts
                                              127 passed, 0 failed
npx tsc --noEmit                              exit 0

New coverage:

  • scope-filter.test.ts — a filterEventsByScope with usage events (#748)
    block mirroring the dashboard cases, including the Windows separator/case
    rules.
  • team-push-interventions.test.ts — a reportUsageToTeam — usage scope isolation (#748) block that seeds one usage.jsonl with events from two
    projects plus one legacy event and asserts the resulting
    stats/<user>.yaml. Both directions are covered: a projectRoot report
    drops the other project's skills, an excludeProjectRoots report drops this
    project's, and a report with no scope option keeps everything (backward
    compatible).

Before the fix the first of those failed with the other project's skill present
in the stats file — the leak itself, reproduced.

End-to-end, real CLI

Built with npm run build and ran the actual binary against an isolated HOME
with two project directories.

Claude-style payload (cwd):

$ echo '{"tool_name":"Skill","tool_input":{"skill":"team-review"},"cwd":"/tmp/e2e/proj-a"}' \
    | node dist/index.js track --stdin --tool claude
$ cat ~/.teamai/usage.jsonl
{"skill":"team-review","timestamp":"...","tool":"claude","cwd":"/tmp/e2e/proj-a"}

Cursor-style payload (no cwd, workspace_roots instead) — the resolver picks
the workspace root:

$ echo '{"tool_name":"Read","tool_input":{"file_path":"/tmp/e2e/other-proj/.cursor/skills/private-thing/SKILL.md"},"workspace_roots":["/tmp/e2e/other-proj"]}' \
    | node dist/index.js track --stdin --tool cursor
$ cat ~/.teamai/usage.jsonl
{"skill":"private-thing","timestamp":"...","tool":"cursor","cwd":"/tmp/e2e/other-proj"}

Slash-command path:

$ echo '{"prompt":"/team-review some args","cwd":"/tmp/e2e/proj-a"}' \
    | node dist/index.js track-slash --stdin --tool claude
$ cat ~/.teamai/usage.jsonl
{"skill":"team-review","timestamp":"...","tool":"claude","cwd":"/tmp/e2e/proj-a"}

(track-slash skips a name that is not on disk, so the skill had to exist under
~/.claude/skills/ before it recorded anything — that check predates this
change.)

Not verified

I did not exercise a real teamai pull against a live git remote, and I did not
run npm run test:e2e — it needs a live test repo I do not have here. The e2e
above covers the write side (the half this change adds); the read side is
covered by the unit tests, which drive the real reportUsageToTeam and assert
the stats file it writes.

Windows: the Windows path rules are asserted as plain strings in
scope-filter.test.ts, so they run on the ubuntu CI too. I developed on
Windows, and npx vitest run there has a set of pre-existing failures
unrelated to this change (a gstack:tdd skill name that is illegal in a Windows
path, scope.test.ts home-dir assertions, and several git-worktree tests). I
confirmed those same failures on a clean checkout of the base commit before
starting.

Docs

docs/designs/team-intelligence-platform.md records the usage.jsonl line
format, so it now shows the cwd field and what the two filters do with an
event that lacks one.

…ver reaches another team's stats

Dashboard events already carry a cwd and are filtered by scope before a
report, but skill-usage events do not, so ~/.teamai/usage.jsonl is
aggregated whole and every project on the machine is shipped to whatever
repo pull happens to be reporting to.

Give UsageEvent an optional cwd, populate it from the hook payload via
the existing resolveHookCwd helper (which already handles Cursor's
workspace_roots), and filter usage with the same filterEventsByScope
dashboard events use. That function is now generic over any event
carrying a cwd, so both call sites share one set of path rules.

Events written before this field have no cwd. A projectRoot report drops
them, since they cannot be shown to belong to that project;
excludeProjectRoots keeps them, since they cannot be shown to belong to
an excluded one. pull reports to both scopes with complementary filters,
so no event is lost from the local queue.

Fixes Tencent#748 (bug 2). Bug 1 of that issue -- the Stop-hook nudge reaching
projects that never initialized teamai -- is left to Tencent#747, which is
already reworking the same gate.
@SaulMoro

Copy link
Copy Markdown
Collaborator

Thanks for picking this up. #753 fixes the same bug 2 of #748 with a different design, so only one of the two can land. I ran one end-to-end script against a build of each.

The script uses the real CLI with an isolated HOME. Project A reports to team-a, project C to team-c, and repo B has no TeamAI. A skill is used in A, then C pulls before A does.

main #750 #753
team-c gets A's skill yes, leak no no
team-a gets A's skill no no, lost yes
B gets the Stop nudge yes yes no

This is how #750 loses the skill:

C pulls
├─ report    filterEventsByScope(events, { projectRoot: C })   A's event dropped
└─ truncate  truncateUsageAfterReport(unfiltered count)         A's event deleted
A pulls      nothing left to report

The PR expects the user-scope target to report what the project target drops. A single pull never has both:

pull (one run, pull.ts)
├─ project config in cwd  → project target only
└─ no project config      → user target, excludeProjectRoots: []  → no filter at all

So a user-scope team would still receive every project's skills. I read that from the code and did not run it.

#753 drops the filter instead. Each scope writes its own <dataHome>/usage.jsonl, and each report reads and truncates only that file. It also fixes bug 1 in the hook dispatcher. #747 leaves that bug as is, because its share gate still returns true with no config.

Would you be OK closing this in favour of #753? If you know a case #753 misses, I'd like to add it there.

@ydflow

ydflow commented Sep 23, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for the thorough review -- and for running the end-to-end script against a build of each. I read the code paths you named and I agree on both counts.

The lost event. My PR takes eventCount from readUsageEvents() unfiltered, reports each target through filterEventsByScope, then calls truncateUsageAfterReport(eventCount) with that unfiltered count. So when C pulls first, A's event is filtered out of the report but still counted as reported, and the truncate removes it anyway -- A's pull then has nothing left. Real bug, and my tests never covered the two-target ordering.

The user-scope hole. pull.ts only fills excludeProjectRoots when projectConfig?.projectRoot exists, so it is [] otherwise, and filterEventsByScope reads an empty array as "no filter". The "user scope excludes project sessions" line in my description is not what the code does -- that was my mistake in writing up the scope of the change.

Closing in favour of #753 is fine by me. I'd rather have the bug actually fixed than my version merged, and the per-scope file also removes the class of bug rather than patching one instance of it.

One note in case it's useful: the part of my diff I'd keep is the cwd field on UsageEvent plus resolveHookCwd at the three write sites (hook-handlers.ts track/trackSlash, usage-tracker.ts trackFromStdin/trackSlashCommand). #753 doesn't need it for per-scope files, but it is still the right field to have on the event if anything downstream ever wants to attribute usage by directory.

I also went looking for a gap in #753 before replying, in case there was a case worth adding there: I suspected the per-scope <dataHome>/usage.jsonl move would orphan the old single ~/.teamai/usage.jsonl for existing users, but saveUserScopeConfig discards it deliberately on first user-scope init and the comment explains why, and your CHANGELOG already states that on a project-only machine pre-upgrade events stay unreported. Both read as intended rather than missed, so nothing from me there.

Thanks for the comparison table -- it made the decision easy.

@ydflow

ydflow commented Sep 23, 2026

Copy link
Copy Markdown
Contributor Author

Closing in favour of #753, as discussed above.

@ydflow ydflow closed this Sep 23, 2026
SaulMoro added a commit to SaulMoro/teamai-cli that referenced this pull request Sep 24, 2026
A scope that never reports (an http source, `usageReport: false`, a
remote rejecting every push) never truncated its usage file, so it grew
without bound. After the report step, pull now keeps each active scope's
newest 5,000 events; a file at or below the cap is not rewritten. The cap
runs after the report's truncate, never between its read and truncate,
where it would shift the deleted lines onto unsent events (Tencent#750).

Every writer of the usage file takes one lock beside it (acquireLock from
update.ts with a bounded retry): hook appends wait at most ~250 ms and
then write anyway, the truncate and the cap wait up to ~5 s. A rewrite
therefore cannot drop an append made while it runs, and two pulls
(http scopes included, which hold no sync lock) cannot interleave their
rewrites. Both rewrites go through a uniquely named temp file beside the
realpath'd file, created with its mode and renamed over it, so a kill or
a full disk leaves the old file whole; the next locked rewrite removes
temps a killed one left behind.
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.

2 participants