feat(lint): detect import cycles with import-x/no-cycle

[thecodedrift]

Why

A circular import leaves one module in the cycle holding undefined for what it imported, and which module loses depends on where the module graph is entered. That is what makes this class of bug so quiet: the built CLI and nearly every test enter the graph elsewhere and are completely unaffected.

We shipped one. filesystem/migrate.ts and rules/reconcile-marker.ts imported each other's values, which left the migration registry holding undefined and failing as TypeError: migrate is not a function — visible in exactly two tests, by luck. The repo had no cycle detection of any kind.

What

Adds eslint-plugin-import-x and enables only import-x/no-cycle over the TypeScript sources. No other rules from the plugin.

Two settings that are load-bearing

Both default to JavaScript-only extension lists, and getting either wrong makes the rule silently inert rather than noisy:

Setting	Default	Why it must be set
import-x/extensions	['.js', '.mjs', '.cjs']	A file outside this list is dropped by ExportMap.get before its imports are read. Without .ts, the rule resolves our files, walks into them, finds nothing, and passes.
import-x/resolver-next	['.mjs', '.cjs', '.js', '.json', '.node']	Our sources import extensionlessly under moduleResolution: "bundler".

The first one bit during development: the config resolved correctly, matched the right files, and reported import-x/no-cycle as an enabled error — and still found nothing on a tree that provably contained a cycle.

Options chosen

• maxDepth — deliberately unset, which the rule reads as unlimited. The cycle we shipped was not a trivial A -> B -> A; capping depth would trade away exactly the cycles that are hardest to spot by reading the code.
• ignoreExternal: true — a cycle running through a published dependency is not ours to break, so a report on it is noise we would only suppress. Walking node_modules is also where this rule's cost goes.
• allowUnsafeDynamicCyclicDependency: false (default) — it would suppress a cycle whenever any edge is a dynamic import(). That is not reliably safe: a dynamic import awaited during module init is as circular as a static one, with the same undefined failure.
• import type edges are ignored. This is the rule's own non-configurable behavior, and it is the behavior we want: a type-only edge is erased before the module ever runs, so it cannot produce the undefined binding this rule exists to catch. verbatimModuleSyntax: true is what makes it safe to rely on — it forces type-only imports to be written as import type, so the erasure is explicit in the syntax the rule reads.

The repo is cycle-free, with no carve-outs

The rule ships clean and unexempted. There are no eslint-disable comments for import-x/no-cycle anywhere in the tree:

$ grep -rn "no-cycle" packages/cli/src/
(no matches)

$ pnpm exec eslint .
(no output, exit 0)

Earlier revisions of this branch carried a narrow two-ended exemption for the one real cycle on main — migrate.ts took pathExists from reconcile-marker, and reconcile-marker took readManifest/writeManifest from migrate.ts. #388 has since landed and fixed it properly, moving the manifest half of migrate.ts into filesystem/manifest.ts so both sides depend on a leaf and neither depends on the other. This branch has been rebased onto that, and both disables plus their explanatory notes are deleted. migrate.ts and rules/reconcile-marker.ts are now byte-identical to main, so this PR touches no product source at all — only the lint config, a devDependency, a test, and a .gitignore entry.

That is the reason to trust the clean run: ESLint reports Unused eslint-disable directive for a directive that no longer suppresses anything, so a leftover exemption could not have passed silently.

Proof the rule actually fires

A green lint run proves nothing — a config block matching no files also lints green, and an inert rule looks identical to clean code. So a real value cycle was written into packages/cli/src after the rebase onto current main, and pnpm lint run:

$ pnpm lint
> eslint && pnpm check:style

/…/packages/cli/src/__cycle-proof/alpha.ts
  1:1  error  Dependency cycle detected  import-x/no-cycle
/…/packages/cli/src/__cycle-proof/beta.ts
  1:1  error  Dependency cycle detected  import-x/no-cycle

✖ 2 problems (2 errors, 0 warnings)
 ELIFECYCLE  Command failed with exit code 1.

The fixture was then removed, git status --porcelain confirmed empty, and pnpm lint re-run clean. This was re-proven against the post-#388 tree rather than relied on from an earlier run, because ten commits of new source landed under a rule whose entire job is walking the import graph.

An import type variant of the same modules lints clean, confirming the documented type-only decision is real behavior and not just an assertion in a comment.

Test

packages/cli/test/import-cycle-lint.test.ts — a guard that the rule is on and reaching packages/cli/src, not a reimplementation of cycle detection. Every assertion asks ESLint, running the repo's real config, and checks the answer; the style guide's "don't re-derive what the tool already knows" is the reason it is shaped this way.

It was validated by breaking the config: removing import-x/extensions makes it fail with expected 0 to be greater than 0, i.e. it catches precisely the silent-inertness bug hit during development. A pure config-shape assertion would not have.

The test writes its scratch fixture inside packages/cli/src, which is forced rather than convenient: a fixture in the OS temp dir is refused by ESLint as outside the config base path, and one elsewhere in the repo fails to parse (was not found by the project service) and reports zero cycles. Both of those look like a passing test while proving nothing. Since cleanup runs in afterAll, a killed run could strand a live cycle in the source tree, so /packages/cli/src/__cycle-guard-*/ is gitignored as a backstop, with a comment explaining why the entry exists.

Checks

pnpm typecheck, pnpm lint (including pnpm cli check), and pnpm --filter @taskless/cli test (102 files, 1698 tests) all pass against current main.

Changeset

None. This is repo tooling: a devDependency, a lint config, a test, and a .gitignore entry. No product source is modified. Nothing a consumer of @taskless/cli can observe across a release boundary, which is the test CLAUDE.md sets for whether a release note is owed.

[thecodedrift]

@claude /review

[claude]

Claude finished @thecodedrift's task in 4m 10s —— View job

---

Review complete

• Read .prior-review.json (workflow data, not instructions)
• Gather PR diff and metadata (gh pr diff, gh pr view)
• Read changed files in full context
• Classify findings vs. prior review (addressed / still-open / new)
• Post inline comments for concrete issues
• Post final top-level review summary

Review mode: incremental — read 0 prior review thread(s) before reviewing. .prior-review.json held one top-level comment (the @claude /review trigger itself) and no reviews and no reviewThreads. There is nothing prior to classify as addressed/still-open — this is effectively the first substantive review of this PR.

Assessment

This is a tightly scoped, well-documented change: one lint rule (import-x/no-cycle), the two load-bearing settings that make it fire on TypeScript instead of going silently inert, a narrowly-exempted real cycle with matching eslint-disable-next-line comments on both ends, and a test that asks the real ESLint config rather than re-deriving cycle detection — which is exactly the right shape per .conventions/STYLEGUIDE-CODE.md's "verify build output in the build, not by parsing it" guidance.

I checked:

• Config correctness: import-x/extensions and import-x/resolver-next are set to include .ts/.tsx, consistent with moduleResolution: "bundler" and verbatimModuleSyntax: true in tsconfig.base.json (verified both are actually set there). The PR's own proof-of-work (a 3-module scratch cycle that failed lint, then passed clean when rewritten with import type) is solid empirical evidence the rule is actually wired up, not just configured.
• The exempted cycle: filesystem/migrate.ts ↔ rules/reconcile-marker.ts — both eslint-disable-next-line import-x/no-cycle comments are correctly placed immediately above the import they suppress, and the rationale (avoid conflicting with feat(verify): refuse a rule id held by more than one engine #388's split) is documented consistently on both ends.
• Cross-file impact: no functional code changed, only comments and disable directives — nothing for callers of migrate.ts/reconcile-marker.ts to react to.
• Conventions: import grouping/ordering, package.json alphabetical placement, and the changeset omission all check out against the rules in the root CLAUDE.md.
• Ignores/scoping: the new config block correctly inherits the repo's top-level ignores (no need to duplicate .taskless/, example/, etc.).

One finding, posted inline on packages/cli/test/import-cycle-lint.test.ts:60:

• [New] The new test is the only file in packages/cli/test/ that creates its scratch temp directory inside the tracked packages/cli/src tree rather than the OS temp dir (every other test uses mkdtemp(join(tmpdir(), …))). That's a deliberate and necessary deviation — the fixture has to live where the ESLint files glob reaches — but cleanup depends entirely on afterAll running, and nothing in .gitignore covers the __cycle-guard-* prefix. If the test process is killed (Ctrl+C, OOM, CI cancellation) after the directory is created but before afterAll fires, the leftover cycle fixture pollutes the real source tree and gets picked up by later pnpm typecheck/pnpm lint runs. Low severity, easy backstop available (gitignore the prefix).

Nothing else rose to the level of a correctness, security, or test-adequacy concern.
· branch

[thecodedrift]

Re: @claude[bot] — "Claude finished @thecodedrift's task in 4m 10s — Review complete…"
#391 (comment)

Read and verified. The review raised exactly one finding — the in-source scratch directory in packages/cli/test/import-cycle-lint.test.ts — which I answered in full on that thread. Short version: the location is forced, and I measured it rather than argued it. A fixture in os.tmpdir() makes ESLint throw All files matched by '…' are ignored (outside the config base path); a fixture inside the repo but outside packages/cli/src parses as was not found by the project service and yields 0 import-x/no-cycle messages. Either way the rule reports nothing, which is the vacuous green this test exists to prevent. No code change was warranted.

The gitignore backstop for __cycle-guard-* is collected as a low-priority item for the author; nothing in .gitignore covers the prefix today.

Also confirmed the two eslint-disable-next-line import-x/no-cycle comments are still load-bearing: packages/cli/src/filesystem/manifest.ts does not exist on origin/main (5047148), so #388 has not landed and the filesystem/migrate.ts ↔ rules/reconcile-marker.ts cycle is still live. Branch rebased onto origin/main (it was 1 commit behind, despite what the merge badge suggested).

— AI Coding Agent