mirror of
https://github.com/Tencent/teamai-cli.git
synced 2026-10-02 03:14:40 +08:00
fix(skills): the gate reads the session's directory, and no project config 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 #753 does.
This commit is contained in:
@@ -94,7 +94,7 @@ path (measured here from a 77-character one).
|
||||
blocks instead (`blockedBy: "config"`), since recall and the source are then
|
||||
unknown and the workflow would fail at `teamai contribute` — a project config
|
||||
too, which detection alone would skip in favour of the user config
|
||||
(`findUnreadableProjectConfig`). The refusal then says what failed (for a file
|
||||
(`findUnreadableProjectConfig`), including one that is not `scope: project`. The refusal then says what failed (for a file
|
||||
that does not parse, which file and where; for one that fails validation,
|
||||
which field and why), since nothing else reports it. The
|
||||
Stop-hook share reminder asks the same gate (`contributeHintAllowed`, called
|
||||
|
||||
@@ -150,6 +150,30 @@ describe('findUnreadableProjectConfig', () => {
|
||||
expect(problem).not.toContain('\n');
|
||||
});
|
||||
|
||||
it('names a project config that is not scope: project, which detection alone skips', async () => {
|
||||
// `scope` omitted defaults to user: detection would read past the file to
|
||||
// the user config, another team's.
|
||||
const configPath = path.join(dir, '.teamai', 'config.yaml');
|
||||
fs.mkdirSync(path.dirname(configPath), { recursive: true });
|
||||
fs.writeFileSync(configPath, 'repo:\n localPath: /x\n remote: https://example.test/a.git\nusername: t\n');
|
||||
|
||||
const problem = await findUnreadableProjectConfig(dir);
|
||||
expect(problem).toContain(configPath);
|
||||
expect(problem).toContain('scope: project');
|
||||
});
|
||||
|
||||
it('does not name the user config when the directory is HOME itself', async () => {
|
||||
// Run from HOME, `<cwd>/.teamai/config.yaml` is the user config, not a project's.
|
||||
vi.stubEnv('HOME', dir);
|
||||
vi.stubEnv('USERPROFILE', dir);
|
||||
const configPath = path.join(dir, '.teamai', 'config.yaml');
|
||||
fs.mkdirSync(path.dirname(configPath), { recursive: true });
|
||||
fs.writeFileSync(configPath, 'repo:\n localPath: /x\n remote: https://example.test/a.git\nusername: t\nscope: user\n');
|
||||
|
||||
expect(await findUnreadableProjectConfig(dir)).toBeNull();
|
||||
vi.unstubAllEnvs();
|
||||
});
|
||||
|
||||
it('names a broken partition config even when the legacy .teamai/ config behind it loads', async () => {
|
||||
// The partition is authoritative; detection skips it when broken and lands
|
||||
// on the legacy config, which may belong to another team.
|
||||
|
||||
@@ -67,14 +67,15 @@ function runContributeCheck(
|
||||
homeDir: string,
|
||||
stdinPayload: string,
|
||||
tool = 'claude',
|
||||
processCwd = homeDir,
|
||||
): Promise<{ stdout: string; stderr: string; code: number }> {
|
||||
return new Promise((resolve) => {
|
||||
const child = execFile(
|
||||
'node',
|
||||
[CLI_PATH, 'contribute-check', '--stdin', '--tool', tool],
|
||||
{
|
||||
// cwd too: the share gate reads a project config under it.
|
||||
cwd: homeDir,
|
||||
// The directory the hook process starts in, which the gate must not read.
|
||||
cwd: processCwd,
|
||||
env: { ...process.env, HOME: homeDir, TEAMAI_LOG_LEVEL: 'silent' },
|
||||
timeout: 10000,
|
||||
},
|
||||
@@ -250,6 +251,20 @@ describe('contribute-check E2E', () => {
|
||||
expect(result.stdout).toBe('');
|
||||
});
|
||||
|
||||
it('never asks the gate about the launcher directory, even when the payload cwd no longer exists', async () => {
|
||||
// The hook process starts in a project whose config does not parse; the
|
||||
// session ran in a worktree since deleted, which holds no project config,
|
||||
// so the user config (recall on) decides.
|
||||
const launcher = path.join(tmpHome, 'launcher');
|
||||
fs.mkdirSync(path.join(launcher, '.teamai'), { recursive: true });
|
||||
fs.writeFileSync(path.join(launcher, '.teamai', 'config.yaml'), 'repo: [unclosed\n');
|
||||
writeEventsFile(tmpHome, buildRichSessionEvents(SESSION_ID));
|
||||
|
||||
const result = await runContributeCheck(tmpHome, makeStdinPayload(SESSION_ID, path.join(tmpHome, 'deleted-worktree')), 'claude', launcher);
|
||||
expect(result.code).toBe(0);
|
||||
expect(result.stdout).not.toBe('');
|
||||
});
|
||||
|
||||
it('produces no output for a trivial session below threshold', async () => {
|
||||
writeEventsFile(tmpHome, buildTrivialSessionEvents(SESSION_ID));
|
||||
|
||||
|
||||
@@ -10,6 +10,7 @@ const { autoDetectInit, findUnreadableProjectConfig, logDim, logError, NotInitia
|
||||
vi.mock('../config.js', () => ({
|
||||
autoDetectInit,
|
||||
findUnreadableProjectConfig,
|
||||
requireInit: vi.fn(),
|
||||
NotInitializedError,
|
||||
BROKEN_CONFIG_ADVICE: 'Fix the file, or move it aside and run `teamai init` to write a new one.',
|
||||
}));
|
||||
|
||||
+24
-3
@@ -1,5 +1,6 @@
|
||||
import YAML from 'yaml';
|
||||
import { ZodError } from 'zod';
|
||||
import fs from 'node:fs';
|
||||
import path from 'node:path';
|
||||
import {
|
||||
TeamaiConfigSchema,
|
||||
@@ -432,7 +433,15 @@ export async function readConfigFrom(
|
||||
try {
|
||||
const raw = YAML.parse(content);
|
||||
const config = LocalConfigSchema.parse(raw);
|
||||
if (config.scope !== 'project') return null;
|
||||
if (config.scope !== 'project') {
|
||||
// Run from HOME, `<cwd>/.teamai/config.yaml` is the user config itself.
|
||||
// Anywhere else a config here that is not scope: project cannot say which
|
||||
// project it serves, and detection would read past it to the user config.
|
||||
if (!isUserConfigFile(configPath)) {
|
||||
onUnreadable?.(configPath, `it is scope: ${config.scope}, but a config inside a project must be scope: project`);
|
||||
}
|
||||
return null;
|
||||
}
|
||||
// Anchor projectRoot to the workspace root (resource landing) and dataHome to
|
||||
// the directory this config lives in (machine-data location). A persisted
|
||||
// projectRoot can be wrong (e.g. a `.teamai/` copied from the main checkout
|
||||
@@ -462,6 +471,18 @@ export async function readConfigFrom(
|
||||
}
|
||||
}
|
||||
|
||||
/** Whether `configPath` is the user config, comparing real paths (a tmp HOME and the cwd can differ by a symlink). */
|
||||
function isUserConfigFile(configPath: string): boolean {
|
||||
const real = (p: string): string => {
|
||||
try {
|
||||
return fs.realpathSync(p);
|
||||
} catch {
|
||||
return path.resolve(p);
|
||||
}
|
||||
};
|
||||
return real(configPath) === real(expandHome(getUserConfigPath()));
|
||||
}
|
||||
|
||||
/**
|
||||
* One line naming what is wrong. A Zod message is a JSON dump of its issues,
|
||||
* whose first line is `[`; each issue's field and reason is what a member fixes.
|
||||
@@ -513,8 +534,8 @@ export async function requireInitForScope(
|
||||
* If cwd has a project-scope config, uses that; otherwise falls back to user scope.
|
||||
* This is the recommended entry point for commands that support both scopes.
|
||||
*/
|
||||
export async function autoDetectInit(): Promise<TeamaiInit> {
|
||||
const projectConfig = await detectProjectConfig();
|
||||
export async function autoDetectInit(cwd?: string): Promise<TeamaiInit> {
|
||||
const projectConfig = await detectProjectConfig(cwd);
|
||||
if (projectConfig) {
|
||||
const teamConfig = await loadTeamConfig(projectConfig.repo.localPath);
|
||||
if (!teamConfig) return throwTeamConfigMissingOrInvalid(projectConfig.repo.localPath);
|
||||
|
||||
+3
-10
@@ -711,17 +711,10 @@ export async function contributeCheck(toolArg?: string): Promise<void> {
|
||||
}
|
||||
|
||||
// The same gate as the dispatcher's handler: hooks written before it still
|
||||
// call this command, and must not nudge towards a `share` that refuses. The
|
||||
// gate reads the cwd, so move to the session's, as hook-dispatch does.
|
||||
if (stdinData.cwd) {
|
||||
try {
|
||||
process.chdir(stdinData.cwd);
|
||||
} catch (e) {
|
||||
log.debug(`contribute-check: chdir to ${stdinData.cwd} failed: ${e instanceof Error ? e.message : String(e)}`);
|
||||
}
|
||||
}
|
||||
// call this command, and must not nudge towards a `share` that refuses. It
|
||||
// is asked about the session's cwd, never the one this process started in.
|
||||
const { contributeHintAllowed } = await import('./skill-content.js');
|
||||
if (!(await contributeHintAllowed())) return;
|
||||
if (!(await contributeHintAllowed(stdinData.cwd))) return;
|
||||
|
||||
const { stopStdoutUnsupported } = await import('./utils/tool-names.js');
|
||||
const tool = toolArg?.toLowerCase() ?? 'claude';
|
||||
|
||||
+14
-9
@@ -97,21 +97,24 @@ export type TeamDetection =
|
||||
| { kind: 'none' }
|
||||
| { kind: 'unusable'; detail: string };
|
||||
|
||||
export async function detectTeam(): Promise<TeamDetection> {
|
||||
const { autoDetectInit, findUnreadableProjectConfig, NotInitializedError, BROKEN_CONFIG_ADVICE } =
|
||||
export async function detectTeam(cwd?: string): Promise<TeamDetection> {
|
||||
const { autoDetectInit, findUnreadableProjectConfig, requireInit, NotInitializedError, BROKEN_CONFIG_ADVICE } =
|
||||
await import('./config.js');
|
||||
// Loading the config can migrate it and say so with `log.info`. That line
|
||||
// must not land in the skill content, the JSON these commands print on
|
||||
// stdout, or a hook's reply, so config loading reports on stderr here.
|
||||
const previous = setStderrOnly(true);
|
||||
try {
|
||||
const unreadable = await findUnreadableProjectConfig();
|
||||
// A directory that no longer exists (a hook payload naming a deleted
|
||||
// worktree) holds no project config, and git refuses to open it.
|
||||
if (cwd !== undefined && !(await pathExists(cwd))) return { kind: 'team', init: await requireInit() };
|
||||
const unreadable = await findUnreadableProjectConfig(cwd);
|
||||
if (unreadable) {
|
||||
// A parse error spans several lines (a code frame); its first names the
|
||||
// file, the line and the column, which is what the member acts on.
|
||||
return { kind: 'unusable', detail: `${firstLine(unreadable)}. ${BROKEN_CONFIG_ADVICE}` };
|
||||
}
|
||||
return { kind: 'team', init: await autoDetectInit() };
|
||||
return { kind: 'team', init: await autoDetectInit(cwd) };
|
||||
} catch (e) {
|
||||
if (e instanceof NotInitializedError) return { kind: 'none' };
|
||||
return { kind: 'unusable', detail: firstLine(e instanceof Error ? e.message : String(e)) };
|
||||
@@ -131,9 +134,9 @@ export async function detectTeam(): Promise<TeamDetection> {
|
||||
* writable, is then unknown, and the workflow would fail at `teamai contribute`.
|
||||
* Any failure past loading the config is a fault here and propagates.
|
||||
*/
|
||||
export async function shareGate(): Promise<ShareGate> {
|
||||
export async function shareGate(cwd?: string): Promise<ShareGate> {
|
||||
const { isRecallEnabled } = await import('./types.js');
|
||||
const team = await detectTeam();
|
||||
const team = await detectTeam(cwd);
|
||||
if (team.kind === 'none') return { block: null, config: null };
|
||||
if (team.kind === 'unusable') return { block: { reason: 'config', detail: team.detail }, config: null };
|
||||
const { localConfig, teamConfig } = team.init;
|
||||
@@ -158,13 +161,15 @@ function firstLine(text: string): string {
|
||||
*
|
||||
* The reminder routes to `share`, so it is withheld wherever `shareGate`
|
||||
* blocks it: a nudge there would send the agent to a command that says no.
|
||||
* Both the hook dispatcher and the legacy `teamai contribute-check` ask this.
|
||||
* Both the hook dispatcher and the legacy `teamai contribute-check` ask this:
|
||||
* the dispatcher has changed into the session's cwd already, the legacy
|
||||
* command passes it, since its process may start anywhere.
|
||||
*/
|
||||
export async function contributeHintAllowed(): Promise<boolean> {
|
||||
export async function contributeHintAllowed(cwd?: string): Promise<boolean> {
|
||||
const { isContributeHintEnabled } = await import('./types.js');
|
||||
let gate: ShareGate;
|
||||
try {
|
||||
gate = await shareGate();
|
||||
gate = await shareGate(cwd);
|
||||
} catch (e) {
|
||||
// A fault in the gate itself, not a config it could not load (the gate
|
||||
// answers that): a Stop hook must not fail the turn over a reminder.
|
||||
|
||||
Reference in New Issue
Block a user