fix(dry-run): thread { dryRun } through the loaders pull, push, status and list use - #866
Conversation
…s and list use `--dry-run` is documented as previewing without making changes, and Tencent#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-Tencent#546 partition or run the single-repo self-heal bootstrap. The loaders already take LoadOptions — Tencent#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 Tencent#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-Tencent#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 Tencent#850
|
Resolves both P1 findings on this PR. pull.ts:1891 - the previewed self-mode config reached lockScope(), whose acquireLock() calls ensureDir() on the lock's parent. On a fresh clone the partition does not exist yet, so the directory was created and stayed: releaseLock removes the lock file, not its parent. The guard sits inside lockScope(), the one choke point all three call sites share. push.ts:731 - the same previewed config ran the whole self-mode setup before pushCore reached its own dry-run guard at push.ts:1577: the sync-lock, migrateSelfModeGitignore(), and the disposable knowledge worktree. Both guards are deliberately narrow. A blanket early return before the git-mode branch would also skip resetToCleanMaster/pullRepo (push.ts:934), which a dry run performs on purpose so it can name the destination the real command would use. Only writes that outlive the command are gated. The preview still reads the uncommitted teamai.yaml that pushCore receives as initialPendingTeamConfig; that block is now pendingSelfTeamConfig(), the identical read, so the preview keeps describing the config edit it exists to describe. Fixture gap, also flagged: dry-run-load-path.test.ts already had a fresh-self-mode-clone fixture, but only tags/roles ran against it. pull and push ran at user scope, or on a project partition that already exists - never on the one shape where acquireLock has something new to create. Two cases added there; the unfixed tree fails them at fs.existsSync(<HOME>/.teamai/projects) with "expected true to be false". Not fixed here, and named in the PR description: pull --dry-run on a fresh clone still creates an empty <HOME>/.teamai/locks/, via listPendingForInstall in utils/pending-learnings.ts. That call is unchanged by this PR and the file is outside its scope; the test declares and counts the entry, so anything else appearing still fails.
Fixes the red Lint & Test on the previous head (all four matrix entries).
pull-scope-isolation.test.ts asserted the exact argument list of
loadLocalConfigForScope, which this change widens to carry LoadOptions:
expect(loadLocalConfigForScope).toHaveBeenCalledWith('user');
received ['user', undefined, { dryRun: undefined }]
The shape is not a free choice: recall.ts:464 and recall.ts:515 already pass
the same third argument, landed with Tencent#853, so pull.ts matches the merged
precedent. recall-scope-isolation.test.ts never asserts the argument list,
which is why the same change left it green.
The affected set is derived from the changed symbols rather than from the
topic: every test file that mentions loadLocalConfigForScope,
detectProjectConfig or autoDetectInit (76 files, 1221 tests).
The two findings from the earlier review are resolved: the project partition lock directory is no longer created, and self-mode push preview no longer runs the gitignore migration or disposable worktree setup. The PR description includes sufficient representative real-CLI verification. |
…g it `acquireLock` is not a read. It `ensureDir`s the lock's parent, which on a fresh self-mode clone is a `<getDataHome>` partition that does not exist yet — and `releaseLock` removes the lock FILE, not that directory, so the directory outlives the command. Any preview that calls it therefore writes, which is the defect this PR is about (Tencent#866). The new `options.dryRun` returns what the preview actually owes its caller: the ANSWER the real run would get. `lockState` already separates `live` (a holder is running) from `stale` and `missing`, and the real run reclaims either of the latter and wins, so `acquireLock(path, { dryRun: true })` is exactly that verdict — no mkdir, no lock file, no reclaim sentinel. Nothing is recorded in `heldLockOwners`, which is what makes the preview safe alongside the existing `releaseLock` calls: it returns at its first line when it holds no owner token for the path, so a preview cannot delete a lock another process owns. No caller passes `dryRun` yet; this commit is the primitive only.
…g what it reports Supersedes the guards added in cfd7c57. Those guards stopped the writes, and E2E (fork-safe) caught what they cost — 6 failures in push-sync-followups-823.test.ts, all of the same shape: expected '- Scanning local resources...\nNo new or modified resources to push' to contain '[rules] teamai-rule (modified)' A preview that reports no changes for a tree with a deliberately edited team rule is not a conservative preview; it is this PR's own defect with the sign flipped. pull.ts — `lockScope()` returned `true` outright for a dry run, asserting the scope was uncontended. That is a fabricated fact: its callers read `true` as "you hold the lock". It now acquires through the read-only primitive and records the lock for release only when it really took one, so a scope with a live holder is reported as contended and skipped, exactly as a real pull does. push.ts — the self-mode branch returned early for a dry run. Two of the three things it skipped are right to skip and one is not. - the sync-lock: read-only now, at both push.ts:784 (self) and push.ts:839 (git mode). - `migrateSelfModeGitignore()`: still skipped. It rewrites a tracked file in the user's ACTIVE tree, which outlives the preview, and it is idempotent, so the next real push performs it. - the knowledge worktree: MUST run, and that is the correction. `pushCore` adds the active tree's `.teamai/{skills,rules}` as scan sources and diffs them against `localConfig.repo.localPath` (push.ts:1095-1112) — the clean worktree checkout. Outside the worktree those are the same path in self mode, so the diff is empty by construction: skipping the worktree does not report an edit early, it hides the edit. `withKnowledgeWorktree` already removes it in a `finally`, so it stays disposable. Fixture — `push --dry-run` now performs its `git fetch` for real, which leaves `app/.git/FETCH_HEAD` behind. Declared and counted next to the existing `<getDataHome>/locks/` entry, so any OTHER new entry still fails the case.
|
|
addressed the four P1 findings from the latest review. All four are one mistake in different places: the preview loads a config that is structurally identical to a real one, and the code that then acts on what it loaded did not know it was looking at a preview. Committed as
|
|
Correction to my comment above — it opens with a missing word (" addressed" should read "Addressed"). The content is otherwise as posted. Also worth stating plainly, since it is the part a reader has to decide on: I pushed to
The commit is a single change on top of your head |
|
@ydflow — #866 was squash-merged 91 minutes after your comment, without
I measured your commit; it does what it says Pulled the 5 files and checked them against the commit's blob shas — all match,
14/14 assertions, including the two that decided Rev2/Rev3. This is strictly better than what I have: Rev5 gates the None of it is in main Second column above. The lock is created at Rebase, not merge — conflict map git's own merge engine, base =
// current main
const queue = await publishQueuedLearnings(localConfig, localConfig.username, { holdsSyncLock: true, dryRun: options.dryRun });
if (options.dryRun) {
if (queue.remaining > 0) log.info(`[${scopeLabel}] [dry-run] Would publish ${queue.remaining} queued learning(s)`);
} else if (queue.published.length > 0) {// df2d1bb
const queue = await publishQueuedLearnings(localConfig, localConfig.username, {
holdsSyncLock: true,
...(options.dryRun ? { dryRun: true } : {}),
});
if (queue.published.length > 0) {Take main's side. Your conditional spread was correct against the branch's signature (
One more, mine, not yours
My proposal Open a new PR against If you would rather not spend that rewrite on a file you did not break, say so and I will take Either way I will verify the landed result the same way I verified |
…Tencent#866) Tencent#866 threaded { dryRun } through the loaders pull, push, status and list use. The scope lookups this branch added did not take it, so `teamai --dry-run env list|set|unset|add|remove` and `env exec --dry-run` still persisted the legacy role migration, adopted a pre-Tencent#546 partition or ran the self-mode bootstrap. - resolveConfigForDir takes LoadOptions and passes them to both loaders. - scopeHere/requireScope (env commands) and commandEnvironment (env exec) forward options.dryRun. Without the flag nothing changes: env exec still runs the migrations every command runs (spec Tencent#879 Conflict 12). Five cases added to dry-run-load-path.test.ts; each failed before this change (config.yaml rewritten, partition renamed).
env list only reads, so like status and list since Tencent#866 it never migrates the config it loads, with or without --dry-run. The dry-run load-path test now runs env list without the flag, which is the case that used to migrate.
What this fixes
Fixes #850.
--dry-runis documented as "Preview mode, no changes made", and #837 made that true fortags subscribe,tags unsubscribeandroles set.pull,pushandstatusstill wrote: the scope-detection block each runs before any dry-run guard called config loaders that were never given{ dryRun }.The issue located the writes by code path and disclosed that the project-scope half — partition adoption and the self-heal bootstrap — had not been run. It is run below.
@ydflow's #853 landed the loader half (
loadLocalConfigForScopegainedLoadOptions) together with thecontribute/session save/recallcall sites. This PR is the remaining command-side half and nothing else.Change
The loaders already accept
LoadOptions; these commands simply did not supply the flag.pull.ts:1891detectProjectConfig(undefined, sink)…(undefined, sink, { dryRun: options.dryRun })pull.ts:1924loadLocalConfigForScope('user')…('user', undefined, { dryRun: options.dryRun })push.ts:731autoDetectInit()autoDetectInit(undefined, { dryRun: options.dryRun })status.ts:52autoDetectInit()autoDetectInit(undefined, { dryRun: true })status.ts:259(list)autoDetectInit()autoDetectInit(undefined, { dryRun: true })pullandpushcarry their own--dry-run.statusandlistpass{ dryRun: true }unconditionally, as you asked in #850: they are read-only, so rather than reading the global flag they take the preview path every time — a read command should never migrate, adopt a partition or bootstrap. Callers that pass nothing behave as before, the same compatibility promise #837 made.Revision 2 — what the first head got wrong
The first head was red on
Lint & Test, all four matrix entries. Two assertions insrc/__tests__/pull-scope-isolation.test.tswere checking the exact argument list of the internal call this change widens:Those two lines now name the third argument, and the source shape is not a free choice:
recall.ts:464andrecall.ts:515already call both loaders exactly this way —(undefined, sink, { dryRun: options.dryRun })and('user', undefined, { dryRun: options.dryRun })— landed with #853. So the call sites match the merged precedent rather than inventing a shape.recall-scope-isolation.test.ts, the parallel test of the parallel command, never asserts the argument list, which is exactly why the same change did not break it.Revision 3 — the two P1 findings
Both findings are correct, and they are one mistake in two places. The preview path returns a config that is structurally identical to a real one:
detectProjectConfig/autoDetectInitunder{ dryRun }hand back the config a bootstrap would have written, and nothing downstream can tell them apart. Revision 1 fixed which options the loaders get; it did not fix the two places that then act on what they loaded.pull.ts:1891→lockScope()(pull.ts:1857)acquireLock(<getDataHome>/.sync-lock)→ensureDir(path.dirname(...))(update.ts:392)releaseLockdeletes the lock file, not its parentpush.ts:731→ thekind === 'self'branch (push.ts:774)acquireLock→migrateSelfModeGitignore()→withKnowledgeWorktree(), all beforepushCore's own guard atpush.ts:1577.teamai/.gitignore, and a git worktree add/remove cycleThe fixture gap, which is the third time I picked a fixture wrongly
Correct finding, and worth naming properly: I chose the fixture from what I had rather than from the path the bug lives on.
dry-run-load-path.test.tsalready had a fresh-self-mode-clone fixture — but onlytagsandrolesran against it;pull/pushran at user scope, or on a project partition that already exists. A fresh clone is the one shape whereacquireLockhas something new to create.Two cases added on that fixture. Same file, same fixture, single variable = the source under test:
The unfixed failure is
expected true to be falseonfs.existsSync(<HOME>/.teamai/projects)— the reported symptom asserted by name, so it cannot pass for an unrelated reason. Nothing that already existed may be rewritten either, and the provider recorder must stay empty.Revision 4 — what Revision 3 got wrong, and CI saying so
Revision 3 shipped a guard at each call site:
lockScope()returnedtrueoutright under a dry run, and the wholekind === 'self'branch returned early. That stopped every write, and it was still wrong.E2E (fork-safe, no credentials)reported it.src/__tests__/e2e/push-sync-followups-823.test.tsfailed 6 of 19, every failure the same shape:A preview that reports "No new or modified resources to push" for a tree with a deliberately edited team rule is not a conservative preview — it is the same defect the PR exists to remove, with the sign flipped: it silently under-reports.
The cause is in
pushCore's own comment (push.ts:1095-1112), which describes what the worktree is for:Outside the worktree,
projectRootandrepo.localPathare the same directory in self mode, so that diff is empty by construction. The worktree is not a side effect of pushing — it is the only source of the clean baseline the scanners compare against. Skipping it does not report an edit early; it hides the edit.The fix, moved down to the primitive
The mistake was guarding at the call site.
lockScope()returns a boolean its caller reads as fact — "do you hold the lock?" — so making it returntrueunder a dry run fabricated an assertion rather than skipping a write. Same shape atpush.ts, where the guard was wide enough to take the baseline with it.So the guard is gone; the primitive answers instead.
acquireLockgains an optionaloptions: { dryRun }(src/update.ts), and under it asks the lock for its state instead of taking it:lockStatealready separateslivefromstale/missing, and a real run reclaims either of the latter and wins — so the preview returns the answer the real command would have got. A scope or project with a live holder is now reported as contended, exactly as a realpull/pushreports it, instead of being called uncontended.ensureDir, no lock file, no reclaim sentinel.releaseLockis safe to pair with it: it returns early when it holds no owner token for the path, so a preview cannot delete a lock another process owns.migrateSelfModeGitignore()rewrites a tracked file in the user's active tree, which outlives the preview; it is idempotent, so the next real push performs it. The git-mode clone refresh (resetToCleanMaster+pullRepo) keeps running, as in Revision 3 — that is what lets the preview name the destination the real command would use.sync-lockatpush.ts:839gets the same read-only treatment as the self-mode one.The self-mode preview still needs the one read the worktree used to supply: the uncommitted
teamai.yamlthatpushCorereceives asinitialPendingTeamConfig. That block ispendingSelfTeamConfig()— the identical read, unchanged for the real path, called by the preview too.The
<getDataHome>/locks/entry in the fixture is unchanged and still declared, because the primitive was not the thing creating it — see the next section.One thing this revision does not fix, deliberately
pull --dry-runon a fresh self-mode clone still creates one empty directory:<HOME>/.teamai/locks/. Measured, not inferred — one probe, run unchanged against both trees:It comes from
listPendingForInstall(utils/pending-learnings.ts:183), whichpublishQueuedLearningscalls to count the queue sopullcan report how many learnings it would publish — andwithQueueLock→acquireLockcreates the lock's parent, on a call that passes nodryRun. It is pre-existing on the base commit as well; it became reachable on a fresh clone only now that detection stops aborting first.I left it alone on purpose: that file is not part of this PR, and main rewrote it in #838 — the same commit that added the
dryRunflag stopping just short of this call. Fixing it here means either shipping a copy of that file carrying #838's refactor, or editing the exact region #838 added; both turn this PR into a merge conflict with main. The test declares the entry and counts it (expect({ appeared, vanished }).toEqual({ appeared: allowed, vanished: [] })), so anything else appearing still fails the case. Worth a follow-up issue of its own — say the word and I will open one.push --dry-runlikewise leaves oneapp/.git/FETCH_HEAD: the preview now performs itsgit fetchfor real, which is the point. It names no ref, changes no working tree, and git overwrites it on the next fetch. Same treatment — declared and counted.Verification
Static checks
The primitive was reviewed against its two callers before pushing, because both could have been silent breakage:
lockState(update.ts:246) is read-only — onereadFile, no branch that writes. The dry-run return is the first statement afterresolvedis computed, so noensureDir, sentinel orheldLockOwnersentry can be reached.heldLockOwners.setappears only on the three real-acquire paths, so under a dry runreleaseLockfinds no owner token and returns at its first line — it cannot delete a stale lock, or anyone else's.Command-level, single-variable control
src/__tests__/dry-run-load-path.test.tsalready carried #837's fixture matrix and #853's loader cases. Seven cases are added, and every one of them fails on unmodifiedmainwith the byte-identical test file — the control tree ismain'ssrcwith this one test file dropped in, so the only variable is the source under test.mainpull --dry-runmigrates nothing it loads (non-git, legacy role config)push --dry-runmigrates nothing it loadsstatusmigrates nothing it loadslistmigrates nothing it loadspull --dry-runadopts no legacy partition on a git projectstatusadopts no legacy partition on a git projectlistadopts no legacy partition on a git projectEach case snapshots every file under the fixture root (minus
debug.logand git's transient locks, as the existing cases do) and asserts the tree is byte-identical afterwards, then asserts the provider recorder saw nothing andupdateReportswas not called.Real CLI, the issue's own reproduction
One isolated
HOMEholding a role-less~/.teamai/config.yamlnext to a team repo whosemanifest/roles.yamldeclareshai; the CLI built from each tree; the only variable is the command.mainpull --dry-run~/.teamai/config.yamlgainsscope: user,additionalRoles: [],primaryRole: hai,resourceProfileVersion: 1statuslist6/6, re-run after Revision 3 against the merge result (
main@5fb316c7+ this change) rather than the earlier snapshot, since a stale e2e is what the guards were added to fix.What I could not adjudicate locally, and what I did instead
This machine blocks synchronous child processes (
spawnSync/execFileSyncanswerEBUSY, including with the sandbox disabled — an antivirus/system layer, not our code).push-sync-followups-823.test.tssets up its fixtures throughexecFileSync('git', …), so every one of its 19 cases dies inbeforeEachwithHook timed out in 15000mshere — exactly the 6 failures this revision is about, from the opposite direction: locally they never even get to assert.That is why the E2E verdict is CI's to give, and why the local evidence above is scoped to what does run:
tsc,oxlint, and the non-E2E set. Of the 27 non-E2E test files that touchacquireLock/releaseLock, the ones that do not also shell out synchronously to git pass. The rest were run on both trees — unmodified base and this revision — and every failure on both is a timeout (Hook timed out in 15000ms/Test timed out in 15000ms), with zero assertion failures on either. The file count that differs between those two runs (5 vs 6 red) is therefore machine scheduling under a hunggit, not this change; and it is the same reason I cannot produce a local verdict onpush-sync-followups-823.test.ts, which is the file that actually discriminates.Scope
init's call sites (it has no--dry-runto honour), the git-mode clone refresh a preview performs on purpose, and the<HOME>/.teamai/locks/directory described above — upstreamlistPendingForInstall, tracked separately rather than patched from here.requireInitForScope(config.ts:625) takes nooptionsand callsloadLocalConfigForScopebare. It has no caller anywhere insrc/, so it reads as exported surface rather than a live path; I left it alone rather than guess at its contract. Worth a follow-up if you want it threaded or removed.