Skip to content

[bug] pull / push / status --dry-run still write: the loaders they use take no { dryRun } #850

Closed

Description

@Smilewithoutfalling

Summary

--dry-run is documented as "Preview mode, no changes made", and #837 made that true for tags subscribe, tags unsubscribe and roles set. It is still not true for pull, push and status: the scope-detection block they run before any dry-run guard calls config loaders that were never given { dryRun }, so a preview still persists the legacy role migration — and, in a git repo, can still adopt a pre-#546 partition and run the single-repo self-heal bootstrap.

#837 named exactly this and left it out:

pull --dry-run, push --dry-run and status reach the same writes through loaders that do not pass { dryRun } (loadLocalConfigForScope, and autoDetectInit/detectProjectConfig without options). They are outside #836 and are not changed here. Callers that pass nothing behave as before.

All line numbers and code below were re-checked on main @ e0bf2e96 (blobs: src/config.ts 04cc88c9, src/pull.ts 2faf8492, src/push.ts 0ad93fd7, src/__tests__/dry-run-load-path.test.ts 247a79a9).

Root cause

#837 threaded LoadOptions into loadLocalConfig, requireInit, autoDetectInit, detectProjectConfig, resolvePartitionDir, readConfigFrom and migrateLegacyRoleConfig. loadLocalConfigForScope was missed, and it calls them bare.

src/config.ts:229 — no options parameter, three bare calls:

export async function loadLocalConfigForScope(
  scope: Scope,
  projectRoot?: string,
): Promise<LocalConfig | null> {
  if (scope === 'project') {
    if (!projectRoot) return null;
    const detected = await detectProjectConfig(projectRoot);                         // :237 no options
    if (!detected) return null;
    return migrateLegacyRoleConfig(detected, path.join(getDataHome(detected), 'config.yaml')); // :239 no options
  }
  const configPath = getConfigPath(scope, projectRoot);
  const content = await readFileSafe(expandHome(configPath));
  if (!content) return null;
  try {
    const raw = YAML.parse(content);
    const parsed = LocalConfigSchema.parse(raw);
    return await migrateLegacyRoleConfig(parsed, configPath);                        // :247 no options
  } catch (e) { … }
}

Callers that run under a dry run:

Caller Line Call Note
src/pull.ts 1885 detectProjectConfig(undefined, sink) options not passed
src/pull.ts 1914 loadLocalConfigForScope('user') the loader takes no options at all
src/push.ts 731 autoDetectInit() options not passed
src/status.ts 46 (and list, :252) autoDetectInit() status(options) already has options in hand

pull() starts at pull.ts:1817. Every options.dryRun guard in that file is later (:1071, :1099, :1119, :1177, …), so the scope-detection block at ~1878–1939 runs first, unguarded.

Bare calls mean options = {}, so inside the loaders:

  • migrateLegacyRoleConfig skips its if (options.dryRun) return migrated early-out and writes config.yaml.
  • resolvePartitionDir (utils/partition.ts:219) skips its dry-run branch (:223) and runs adoptLegacyPartition → fs.promises.rename(legacyDir, canonical) (:237) plus the repo.localPath rewrite that follows.
  • selfHealAndReadPartition (config.ts:470) skips if (options.dryRun) return previewSelfHeal(…) (:476) and runs the real bootstrapSelfRepo (:479) — partition config, state, anchor, tool dirs, hooks, git worktree add, member registration and its push, plus the provider auth calls previewSelfBootstrap deliberately avoids.

The plumbing itself already exists — autoDetectInit(cwd?, options = {}) (config.ts:642) forwards options to detectProjectConfig (:643). These commands simply do not supply their flag.

Reproduction — real CLI, single-variable control

CLI built from unmodified main (npm run build, node dist/index.js). One fixture — an isolated HOME holding a role-less ~/.teamai/config.yaml next to a team repo whose manifest/roles.yaml declares hai — and the only variable is the command. Tree compared by sha256 over every file, the same method as dry-run-load-path.test.ts.

Command Files changed Output
tags subscribe testing --dry-run (fixed by #837 — control) 0 ℹ [dry-run] Would migrate legacy teamai config to default role profile: hai
ℹ [dry-run] Would subscribe to: testing
pull --dry-run 1 — ~/.teamai/config.yaml rewritten ℹ Migrated legacy teamai config to default role profile: hai

~/.teamai/config.yaml after pull --dry-run gains primaryRole: hai, resourceProfileVersion: 1, scope: user and additionalRoles: []. The control command writes nothing, so the harness is not a false negative — it does see writes, and this one is real.

Evidence level. The user-scope write above is run and observed. The project-scope writes in a git repo (partition adoption, self-heal bootstrap) are located by code path from the same unguarded calls — quoted line by line above, not yet run. I will run them before opening a PR.

Proposed fix

Same shape as #837, applied to the loader it missed:

  1. loadLocalConfigForScope(scope, projectRoot?, options: LoadOptions = {}) — pass options into detectProjectConfig and both migrateLegacyRoleConfig calls. Callers that pass nothing behave as before, the same compatibility promise fix(tags,roles): honor --dry-run and count namespaced skills in tags list (#836) #837 made.
  2. pull.ts:1885 and pull.ts:1914 pass { dryRun: options.dryRun }; push.ts:731 passes it to autoDetectInit.
  3. status.ts:46 and status.ts:252: status and list are read-only, so rather than reading the global flag they would pass { dryRun: true } unconditionally — a read command should never migrate, adopt or bootstrap. Happy to use a narrower option name if you prefer one.
  4. Extend src/__tests__/dry-run-load-path.test.ts. COMMANDS is at :142–:146; the three fixtures (:136), the provider-call recorder (:10–:24) and the updateReports spy (:27–:30) already exist. pull and push fit the existing row shape — pull emits [<scope>] [dry-run] Would … (pull.ts:1071, :1099, :1119, :1177), so the shared assertion holds. status needs a second assertion: it prints no [dry-run] Would line today, so its row should assert the absence of writes, not a message.

Scope

  • In: the loader's { dryRun } threading and the three commands; the test matrix.
  • Out: init's call sites (it has no --dry-run to honour), and any change to what a real, non-dry run writes.
  • Not audited, same bare-call shape: requireInitForScope (config.ts:625), recall.ts:513, contribute.ts:197, save-session.ts:102. They may be affected the same way; I would keep this change to the three commands fix(tags,roles): honor --dry-run and count namespaced skills in tags list (#836) #837 named unless you want them swept in.

Questions

  1. Do you want this tracked as a follow-up under oxlint rollout: bugs found in the baseline, and next rules to evaluate #836, or as its own issue? I filed it separately because oxlint rollout: bugs found in the baseline, and next rules to evaluate #836's checklist is the oxlint rule rollout, but it can be folded in.
  2. For status: pass { dryRun: true } unconditionally, or would you rather have a distinct read-only option?
  3. When a read-only command does not write, should it still print a [dry-run] Would bootstrap … preview line for the bootstrap it declined to run, or stay silent?

Happy to take it if it is not already spoken for.

Activity

  1. Smilewithoutfalling commented on Sep 27, 2026

    @Smilewithoutfalling
    ContributorAuthor

    Filed from #837's own out-of-scope note, with the evidence and a proposed shape above.

    @SaulMoro @jeff-r2026 — the two open questions at the bottom are yours to call, and I will not code before you answer:

    1. follow-up under oxlint rollout: bugs found in the baseline, and next rules to evaluate #836, or its own issue;
    2. status/list passing { dryRun: true } unconditionally, versus a distinct read-only option.

    One caveat stated in the report and repeated here: the user-scope write is run and observed, but the project-scope writes (partition adoption, self-heal bootstrap) are located by code path only. I will run those before opening a PR, and I will drop the whole thing if the fix is already in flight.

    Happy to take it.

  2. ydflow commented on Sep 27, 2026

    @ydflow
    Contributor

    Filed the three "not audited, same bare-call shape" commands as #853 — contribute, session save and recall, each loading its config before its own dry-run guard. The loader change is the same one your item 1 proposes (loadLocalConfigForScope gains LoadOptions), so the two PRs apply cleanly in either order — happy to fold this into #850 instead if you'd rather land one PR. No rush on my side either way; your open questions come first.

  3. self-assigned this
    on Sep 27, 2026
  4. SaulMoro commented on Sep 27, 2026

    @SaulMoro
    Collaborator

    Thanks for the detailed write-up. On the two questions:

    1. Its own issue. It covers different commands from oxlint rollout: bugs found in the baseline, and next rules to evaluate #836's linter items, and fix(dry-run): thread { dryRun } through the loaders contribute, session save and recall use (#850) #853 already landed the contribute / session save / recall part under it.
    2. Pass { dryRun: true } unconditionally from status and list. They are read-only, so the preview path is exactly what they need, and fix(tags,roles): honor --dry-run and count namespaced skills in tags list (#836) #837's LoadOptions already carries it. A separate read-only option would duplicate it. The migration then persists on the next command that writes.

    Go ahead whenever you're ready.

  5. added a commit that references this issue on Sep 28, 2026
    b3b3a0b
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Labels

No labels
No labels

Type

No type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions