mirror of
https://github.com/Tencent/teamai-cli.git
synced 2026-10-02 03:14:40 +08:00
fix(dry-run): thread { dryRun } through the loaders pull, push, status and list use
`--dry-run` is documented as previewing without making changes, and #837 made that true for `tags subscribe`, `tags unsubscribe` and `roles set`. `pull`, `push` and `status` still wrote: each runs its scope-detection block before any dry-run guard, and that block called config loaders that were never given the flag. A preview could therefore persist the legacy role migration, and in a git repo adopt a pre-#546 partition or run the single-repo self-heal bootstrap. The loaders already take LoadOptions — #853 threaded them through `loadLocalConfigForScope` for contribute / session save / recall. These commands simply did not supply the flag. - pull.ts: both loaders take { dryRun: options.dryRun }. - push.ts: autoDetectInit takes it. - status.ts and list: { dryRun: true } unconditionally, because both are read-only and should never migrate, adopt a partition or bootstrap. Callers that pass nothing behave as before, the same compatibility promise #837 made. The one observable change is that the preview path logs, so `status`/`list` now surface a "[dry-run] Would ..." line where a migration or bootstrap is pending; the PR description asks for a decision on that label. Verification: seven new command-level cases, each failing on unmodified main with the identical test file (the project-scope three need a git project with a pre-#546 partition name, which the existing user-scope fixture never reaches); and the issue's own real-CLI reproduction, 6/6 — the control writes the reported fields, this change writes nothing. oxlint --deny-warnings and tsc --noEmit: rc=0 here and rc=0 on unmodified main. Closes #850
This commit is contained in:
@@ -31,8 +31,11 @@ vi.mock('../utils/reports-branch.js', async (importOriginal) => ({
|
||||
|
||||
import { contribute } from '../contribute.js';
|
||||
import { loadLocalConfigForScope } from '../config.js';
|
||||
import { pull } from '../pull.js';
|
||||
import { push } from '../push.js';
|
||||
import { recall } from '../recall.js';
|
||||
import { rolesSet } from '../roles-cmd.js';
|
||||
import { list, status } from '../status.js';
|
||||
import { tagsSubscribe, tagsUnsubscribe } from '../tags.js';
|
||||
import { updateReports } from '../utils/reports-branch.js';
|
||||
import { log } from '../utils/logger.js';
|
||||
@@ -236,4 +239,63 @@ describe('--dry-run through the loaders the commands share (#850)', () => {
|
||||
expect(loaded?.primaryRole).toBe('hai');
|
||||
expect(fs.readFileSync(configPath, 'utf-8')).toContain('primaryRole: hai');
|
||||
});
|
||||
|
||||
// The command-level half of #850. Each of these reaches the legacy role
|
||||
// migration through a loader it used to call bare, so the fixture's
|
||||
// `config.yaml` gained `primaryRole` even though nothing had asked to write.
|
||||
// `pull`/`push` carry `--dry-run`; `status`/`list` are read-only and pass it
|
||||
// unconditionally (see the note at their `autoDetectInit` call site).
|
||||
//
|
||||
// The positive control is the test directly above: the SAME fixture does gain
|
||||
// `primaryRole` when the flag is absent, so an unchanged tree here is a real
|
||||
// result and not the harness failing to look.
|
||||
const LOAD_ONLY_COMMANDS: Array<[string, () => Promise<void>]> = [
|
||||
['pull --dry-run', () => pull({ dryRun: true })],
|
||||
['push --dry-run', () => push({ dryRun: true })],
|
||||
['status', () => status({})],
|
||||
['list', () => list(undefined, {})],
|
||||
];
|
||||
|
||||
it.each(LOAD_ONLY_COMMANDS)('%s migrates nothing it loads (#850)', async (_command, run) => {
|
||||
const { root, configPath } = legacyRoot();
|
||||
const before = snapshotTree(root);
|
||||
const error = await run().then(() => null, (e: unknown) => e);
|
||||
expect(error).toBeNull();
|
||||
expect(snapshotTree(root)).toEqual(before);
|
||||
expect(fs.readFileSync(configPath, 'utf-8')).not.toContain('primaryRole');
|
||||
// A dry run may parse the remote, but nothing else may reach a provider.
|
||||
expect(providerCalls).toEqual([]);
|
||||
});
|
||||
|
||||
/** A git project whose partition still carries its pre-#546 name, i.e. project scope. */
|
||||
function projectRoot(): string {
|
||||
const root = fs.mkdtempSync(path.join(os.tmpdir(), 'teamai-dry-run-project-'));
|
||||
roots.push(root);
|
||||
const home = path.join(root, 'home');
|
||||
fs.mkdirSync(path.join(home, '.teamai'), { recursive: true });
|
||||
// An installed agent, so a bootstrap that did run would seed and wire it.
|
||||
fs.mkdirSync(path.join(home, '.claude'), { recursive: true });
|
||||
vi.stubEnv('HOME', home);
|
||||
process.chdir(setupLegacyNamedPartition(root));
|
||||
return root;
|
||||
}
|
||||
|
||||
// The project-scope half. These reach the same bare calls through
|
||||
// `detectProjectConfig`, whose dry-run branch is what stops
|
||||
// `adoptLegacyPartition` (a real `fs.rename`) and the self-heal bootstrap.
|
||||
// The issue report located these by code path only; they are run here.
|
||||
const PROJECT_SCOPE_COMMANDS: Array<[string, () => Promise<void>]> = [
|
||||
['pull --dry-run', () => pull({ dryRun: true })],
|
||||
['status', () => status({})],
|
||||
['list', () => list(undefined, {})],
|
||||
];
|
||||
|
||||
it.each(PROJECT_SCOPE_COMMANDS)('%s adopts no legacy partition on a git project (#850)', async (_command, run) => {
|
||||
const root = projectRoot();
|
||||
const before = snapshotTree(root);
|
||||
const error = await run().then(() => null, (e: unknown) => e);
|
||||
expect(error).toBeNull();
|
||||
expect(snapshotTree(root)).toEqual(before);
|
||||
expect(providerCalls).toEqual([]);
|
||||
});
|
||||
});
|
||||
|
||||
+6
-2
@@ -1882,7 +1882,11 @@ export async function pull(
|
||||
let projectConfig: LocalConfig | null = null;
|
||||
const unreadable: string[] = [];
|
||||
try {
|
||||
projectConfig = await detectProjectConfig(undefined, (configPath, error) => { unreadable.push(`${configPath}: ${error}`); });
|
||||
projectConfig = await detectProjectConfig(
|
||||
undefined,
|
||||
(configPath, error) => { unreadable.push(`${configPath}: ${error}`); },
|
||||
{ dryRun: options.dryRun },
|
||||
);
|
||||
} catch (e) {
|
||||
log.warn(`Project-scope detection error: ${(e as Error).message}`);
|
||||
}
|
||||
@@ -1911,7 +1915,7 @@ export async function pull(
|
||||
log.info('project scope detected, skipped user scope');
|
||||
} else {
|
||||
try {
|
||||
const loadedUserConfig = await loadLocalConfigForScope('user');
|
||||
const loadedUserConfig = await loadLocalConfigForScope('user', undefined, { dryRun: options.dryRun });
|
||||
if (loadedUserConfig) {
|
||||
if (inheritUserScope) {
|
||||
inheritedUserConfig = loadedUserConfig;
|
||||
|
||||
+1
-1
@@ -728,7 +728,7 @@ export async function push(
|
||||
result?: { completed: boolean },
|
||||
): Promise<void> {
|
||||
// Auto-detect scope: project scope if cwd has project config, else user scope
|
||||
const { localConfig, teamConfig } = await autoDetectInit();
|
||||
const { localConfig, teamConfig } = await autoDetectInit(undefined, { dryRun: options.dryRun });
|
||||
assertNotReadOnly(localConfig, 'teamai push');
|
||||
|
||||
// --project is a destination override expressed as a logical project. Each
|
||||
|
||||
+11
-4
@@ -42,8 +42,14 @@ export async function status(options: GlobalOptions): Promise<void> {
|
||||
await statusAll();
|
||||
return;
|
||||
}
|
||||
// Auto-detect scope
|
||||
const { localConfig, teamConfig } = await autoDetectInit();
|
||||
// Auto-detect scope.
|
||||
// This is a read-only command, so `dryRun` is passed unconditionally rather
|
||||
// than forwarded from `options.dryRun`: the load must never migrate a legacy
|
||||
// role config, adopt a pre-#546 partition, or run the self-heal bootstrap
|
||||
// (#850). The preview path returns what a write would have produced, so the
|
||||
// report below still tells the truth, and the migration then persists on the
|
||||
// next command that writes.
|
||||
const { localConfig, teamConfig } = await autoDetectInit(undefined, { dryRun: true });
|
||||
const scopeLabel = localConfig.scope;
|
||||
|
||||
// Scope info
|
||||
@@ -248,8 +254,9 @@ async function statusAll(): Promise<void> {
|
||||
}
|
||||
|
||||
export async function list(type: string | undefined, options: ListOptions): Promise<void> {
|
||||
// Auto-detect scope
|
||||
const { localConfig, teamConfig } = await autoDetectInit();
|
||||
// Auto-detect scope — read-only, so `dryRun: true` unconditionally, as in
|
||||
// `status` above (#850).
|
||||
const { localConfig, teamConfig } = await autoDetectInit(undefined, { dryRun: true });
|
||||
const repoPath = localConfig.repo.localPath;
|
||||
|
||||
const source = options.source ?? 'all';
|
||||
|
||||
Reference in New Issue
Block a user