fix(skills): one share gate, actionable refusals, and a louder stub deploy - #747
Merged
Merged
Conversation
…eploy Follow-ups from the review of Tencent#699: - The Stop-hook reminder and `teamai skill get share` ask one gate (`shareGate`, through `contributeHintAllowed`). The hook skipped the unreadable-project-config check, and the legacy `teamai contribute-check` command, still called by hooks written before the dispatcher, checked nothing, so both nudged towards a command that refused. - The gate reads only a config load failure as "cannot be loaded"; any other fault propagates (the hook withholds the reminder and logs it at debug). - A `config` refusal says what failed (the file and position for a parse error) instead of pointing at `teamai doctor`, which cannot see a broken config. `skill show` now refuses through the same helper, so its hint moves from stdout to stderr like `skill get` and `skill path`. - `pull` warns when the discovery stub cannot be deployed (it was an empty catch on the fast path and a debug line on a full sync), and so does the legacy prune. - Error text no longer claims a reason was logged when none was: an empty config is named as empty, and `init` points at ~/.teamai/debug.log, where every path that deploys nothing now records why. - `core` routes a bare `/teamai` right after a friction reminder to `share`, as the stub already said. - The command drift guard rejects an unknown subcommand inside a group (`teamai skill gett core` passed before). - The contribute-check e2e asserts the reminder's real text again; the usage guides (EN, zh-CN) and the design doc cover the config refusal, the gate and the reminder routing.
A push into the bare origin can leave `git gc --auto` writing to objects/pack after the test returns; the single rmdir in afterEach then fails with ENOTEMPTY (seen on CI, Node 22 ubuntu, Tencent#747).
|
Review of Tencent#747: - `skill show share` under a broken project config searched the user config's team repo and agents, which detection falls back to, and printed a `share` found there. It now asks the gate first and refuses on a config block before any lookup. With an empty user config it refuses instead of ending in a stack trace. - A config that parses but fails validation reported the Zod JSON dump, whose first line is `[`, so the refusal said `config.yaml: [.`. Every config loader now reports each issue as `field: reason` on one line. - The docs and skills that describe the share reminder or the refusal say it is withheld on a read-only source and while the config cannot be loaded, and that a validation failure names the field: product-overview and usage-guide (EN, zh-CN), designs/skill-serving.md, core/SKILL.md, contribute-member, setup-admin, join-member and manage-admin.
Collaborator
Author
|
Addressed in 88829dd:
Left as known issues in Merge Danger: |
Collaborator
Author
|
GitLab leg done: same matrix script against a private throwaway gitlab.com fixture with a PAT, four agents. Every row matches the |
|
Findings
Resolved
Tests were not run, per the review-only instruction. |
`contributeHintAllowed` fell open with no config at all, so a caller other than the dispatcher (the legacy `teamai contribute-check`) still nudged in projects that never set up teamai, which have no team to share with (Tencent#748). It now returns false there. Serving the skill stays fail-open.
`teamai contribute-check --stdin` asked the share gate about the directory the hook process started in, while the session analysis used the payload cwd. Started outside the project, it could read the user config and nudge where `teamai skill get share` refuses (a project config that does not load). It now moves to the payload cwd first, as hook-dispatch does.
The previous commit turned both deploy catches into `log.warn`, which is muted in silent mode and never reaches debug.log, and a SessionStart pull runs detached with its output discarded. So the automatic pull, the one that deploys the stub for most members, lost the only persistent record it had. Both catches now warn and write the same line to debug.log.
Known issues left by Tencent#747: - `skill show <name>` and `skill list` on a config that exists but does not load ended in a Node stack trace, and under a broken project config they searched the user config detection falls back to: another team's repo and agents. Both now ask `detectTeam`, the one place that tells "this team", "no team" and "cannot tell, and why" apart (`shareGate` is built on it). Without a usable team, `show` answers from the package alone and `list` prints only the packaged catalog; both say what failed on stderr and exit 1. - A teamai.yaml that exists but fails validation was reported as "not found. Check your repo path". It is now named as invalid, empty or unreadable, like the local config.
Collaborator
Author
|
Addressed the second review in the commits after 88829dd:
Also in this push:
|
|
Findings
Resolved
Tests were not run, per the review-only instruction. |
…onfig is skipped Codex review of 5793758: - A project-location config that is not `scope: project` (or omits `scope`, which defaults to user) was skipped without a word, so the gate read past it to the user config. It is now reported as unusable, unless it is the user config itself, as when running from HOME. - The legacy `contribute-check` changed into the payload cwd and, if that failed, asked the gate about the directory the process started in. It now passes the payload cwd to the gate (`detectTeam(cwd)`), and a cwd that no longer exists holds no project config, so only the user config is asked, as Tencent#753 does.
`log.warn` wrote to the console only and was muted in silent mode, so a detached SessionStart pull, whose output is discarded, lost every warning: the stub deploy failure and the legacy prune among them. Warnings now reach debug.log like debug and error lines. `warnStubNotDeployed` drops the second `log.debug` call, which printed the line twice under --verbose.
Collaborator
Author
|
Addressed the third review in 79e4432 and b0583f5:
Left for after #753: the dispatcher's |
|
Findings
Resolved
Tests were not run, per the review-only instruction. |
…not HOME Codex review of b0583f5: - The dispatcher's `contribute-check` and `pending-hint` handlers asked the gate about the process's directory, trusting hook-dispatch's `chdir`; when that failed, the launcher's config decided. They now pass `resolveHookCwd(stdin)`, as the legacy command does. - The HOME exception for a non-project scope compared the config file's real path, so a project config symlinked to ~/.teamai/config.yaml passed for the user config. It is now decided by the project's location: its root is HOME.
Collaborator
Author
|
Addressed the fourth review in 15a5b5d:
|
Conflicts resolved as planned before Tencent#753 landed: - hook-handlers.ts: Tencent#753's local `contributeHintAllowed` is dropped; the handlers use skill-content's, with the payload cwd. - hook-handlers.test.ts: both config mocks kept, and this branch's tests. - contribute-check-e2e: the fixture keeps recall on, which the gate needs. - docs and manage-admin: both sentences combined; skill-serving already places `contributeHintAllowed` in src/skill-content.ts. - contribute-check.ts merged cleanly: Tencent#753's `resolveConfigForDir` check and this branch's gate run in turn on the same payload cwd.
|
Findings
Resolved
Tests were not run, per the review-only instruction. |
…t once Codex review of 15a5b5d: - `detectTeam` read any failure to see the payload cwd as "deleted", so a cwd it could not open (no permission, a path through a file) fell back to the user config and could allow the reminder. Only ENOENT does now; anything else is `unusable` and withholds it. - `skill show share` and `skill list` loaded the config twice, through the gate and then the team lookup, and reported a broken one twice. Both detect the team once and hand it to the gate.
b0583f5 made every `log.warn` append to debug.log, wider than the two failures it was for, and it wrote unrelated subprocess errors to disk. `log.warn` is console-only again; `log.persist` writes one line to debug.log and never to the console. The stub deploy catches and the legacy prune catch use both, so a detached SessionStart pull keeps the record and --verbose prints it once.
Collaborator
Author
|
Merged
|
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
2 participants
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.
Follow-ups from the review of #699.
Summary
One place decides which team a directory belongs to, and one gate on top of it decides whether
sharecan run. Every path that servesshare, shows or lists skills, or nudges towardsshareasks them.detectTeamchecks the project config before detection, which skips a broken project file and falls back to the user config: another team's repo, recall and source. A project-location config that is notscope: projectcounts as broken too, unless the project's root is HOME, where that file is the user config. A symlink to the user config does not qualify. Both hooks hand the gate the payloadcwd, so the directory the process started in is never read. Acwdthat no longer exists (ENOENT) holds no project config, so only the user config is asked there, as in #753; one that cannot be opened for any other reason withholds the reminder.detectTeamreads only a failed config load asunusable; any other error propagates, and the hook then withholds the reminder and logs the error at debug.With no config at all, serving stays fail-open (
teamai skill get shareworks on a fresh install), but the reminder is silent. A project that never set up teamai has no team to share with (#748). #753, now merged, filters the dispatcher's team handlers for the same case and checksresolveConfigForDirfirst in the legacy command; the gate here coverscontributeHintAlloweditself on both paths.What a member sees:
teamai doctordoes not report a broken project config, and it calls a broken user config "not initialized", so the refusal no longer sends people there. Every config loader reports a validation failure asfield: reasonon one line instead of Zod's JSON dump, whose first line is[.Smaller changes:
Docs and skills say the reminder is withheld on a read-only source, while the config cannot be loaded, and where teamai is not set up, and that a validation failure names the field. The read-only half was already true before this PR; those lines had only named recall.
Left out on purpose: the Chinese trigger phrases the stub lost in #699. A test and the repo rule forbid Chinese in
skills/andskill-data/, so maintainers should decide that in its own issue.Evidence
Real CLI, per provider, on 84a461d (after merging
mainwith #753).npm run buildpassed. Each leg used an isolated HOME and Claude, Codex, CodeBuddy and OpenCode. Afterpull, each agent'steamai/SKILL.mdis byte-identical to the packaged stub. Each row sets its state, then runsskill get share,skill show share, and a Stop payload throughhook-dispatch stopand the legacycontribute-check --stdin. From the broken-config rows on, the user scope also holds a member's ownshareskill.The same legs also check:
provider: gitinteamai.yaml.initreads afile://URL asowner/repoand picks GitHub, so this leg writes the local config by hand.teamai init https://github.com/<fixture>into a private throwaway repo, read only.teamai init https://gitlab.com/<fixture>into a private throwaway repo, read only, with a PAT (teamai's GitLab client sendsPRIVATE-TOKEN). Same script, run in a separate environment; every row matched.The debug.log record for a failed stub deploy is covered by
loggerandpull-skip-sync. The legs do not force a deploy failure.Before (d51f412, same script, git leg):
Tests. These fail on the commit named and pass now:
New tests also cover a multi-line parse error, a fault past the config load, and
teamai recall enabelstill passing the guard.Merge Danger
Door: two-way
The PR changes messages, a gate, docs and tests. It deletes and migrates nothing.
Blast Radius: reminders
sharerefuses anyway.skill showrefusal hint moves from stdout to stderr.skill showandskill listexit 1 and show only packaged skills, even when a member's own skill exists on the machine.contribute-checkpass the payloadcwdto the gate instead of trusting achdir.scope: project, or symlinked to the user config, now blocksshareand the reminder, andskill show/skill listdo not search its fallback team.initand the self bootstrap always writescope: project, so only a hand-written or foreign file is affected.log.persistis new: one line to~/.teamai/debug.log, never the console. Only the stub deploy catches and the legacy prune catch use it.mainwith fix: keep TeamAI out of projects that never set it up (#748) #753 is merged in (6573bd0). fix: keep TeamAI out of projects that never set it up (#748) #753's localcontributeHintAllowedis dropped in favour of the one insrc/skill-content.ts, and the legacy command runs fix: keep TeamAI out of projects that never set it up (#748) #753'sresolveConfigForDircheck and then this gate, both on the payloadcwd.