feat(lint): migrate to TypeScript 7 and adopt oxlint type-aware linting - #534
Merged
Conversation
Supersedes PR #529's peer-range widening. Pins an explicit typescript@7.0.2 devDependency (previously resolved only transitively), adds oxlint-tsgolint, and enables oxlint's type-aware engine (options.typeAware) with 58 of the 59 currently-implemented typescript/* type-aware rules at "error" (prefer-readonly-parameter-types excluded — not in typescript-eslint's own recommended/strict presets, accounted for ~930 of ~3546 trial findings). This activates typescript/no-deprecated for the first time — it was already configured in .oxlintrc.json but silently inert since type-aware linting was never available before TS7. Fixes every finding the new rules surfaced across src/, tests/, lint/, and the .archgate/adrs/*.rules.ts companion files: precise Mock<T> typing for bun:test spies (replacing untyped ReturnType<typeof spyOn>), real zod-schema narrowing in place of unchecked JSON.parse casts, explicit boolean coercion for nullable env/string checks per ARCH-014, async/await correctness, and narrow justified suppressions only for deliberately-fake test fixtures or genuine tsgolint/tsc parity gaps (documented inline). Scopes shims/ (untyped distribution CJS) and docs/ (separate sub-project, dependencies not installed at root) out of type-aware analysis via .oxlintrc.json overrides/ignorePatterns. Signed-off-by: Claude <noreply@anthropic.com> Signed-off-by: Rhuan Barreto <rhuan@barreto.work>
Accidentally included in the previous commit. Signed-off-by: Claude <noreply@anthropic.com> Signed-off-by: Rhuan Barreto <rhuan@barreto.work>
…hollerith Signed-off-by: Rhuan Barreto <rhuan@barreto.work> # Conflicts: # knip.json
… migration - New ARCH-026 (Type-Aware TypeScript Linting): documents the TypeScript 7 + oxlint-tsgolint adoption decision, the explicit devDependency pinning rationale, the 58-of-59 rule-selection policy (prefer-readonly-parameter-types and require-await excluded, with reasoning), scope exclusions (shims/, docs/, .simple-release.js), and the Mock<T>/async/suppression conventions the fix pass established. Companion rule verifies .oxlintrc.json's options.typeAware and the paired devDependencies stay present. - ARCH-014: adds the Boolean()-as-sole-condition vs eslint(no-extra-boolean-cast) caveat surfaced by enabling strict-boolean-expressions. - ARCH-019: adds a worked example for keeping withPromptFix adjacent to inquirer.prompt() when the prompt body is too long to stay inline (a real false-positive hit during the migration). - Agent memory: records that parallel subagents sharing one working directory (no worktree isolation) can have their uncommitted work silently wiped by any single agent's stray git stash/rebase/reset — observed twice during this session's 12-agent fix campaign. Signed-off-by: Claude <noreply@anthropic.com> Signed-off-by: Rhuan Barreto <rhuan@barreto.work>
…hollerith Signed-off-by: Rhuan Barreto <rhuan@barreto.work> # Conflicts: # .archgate/adrs/ARCH-020-glob-scan-include-dotfiles.rules.ts # .archgate/adrs/ARCH-023-engine-file-listing-via-in-memory-git-tracked-matching.rules.ts
|
Important Review skippedToo many files! This PR contains 193 files, which is 93 over the limit of 100. To get a review, narrow the scope: Upgrade to a paid plan to raise the limit. This review couldn't start because sufficient usage credits or metered capacity aren't available. Add credits or update usage-based reviews in the billing tab, then retry. ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (193)
You can disable this status message by setting the Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Deploying archgate-cli with
|
| Latest commit: |
85dbdbb
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://09620bfc.archgate-cli.pages.dev |
| Branch Preview URL: | https://claude-warm-watching-holleri.archgate-cli.pages.dev |
rhuanbarreto
commented
Jul 26, 2026
rhuanbarreto
left a comment
rhuanbarreto
left a comment
Contributor
Author
There was a problem hiding this comment.
General comment about type guards. This is super brittle. We should actually check if it is instead of a generic object.
…eckout CI failed on this PR with ~1127 findings, every ctx.* access in .archgate/adrs/*.rules.ts resolving to any/error instead of RuleContext. Root cause: .archgate/rules.d.ts is gitignored and only regenerated as a side effect of `archgate check`, but `bun run validate` runs `lint` before `check` — a fresh checkout (any new clone, or CI) has neither the shim nor tsconfig coverage for .archgate/ and scripts/ (neither dir was ever in tsconfig's include), so type-aware analysis of those files silently degrades to `any`. Never caught locally because this session's working directory generated the shim early on and it persisted for the rest of the session, papering over the ordering dependency. Reproduced by cloning the pushed branch fresh and running `bun install --frozen-lockfile` there, matching CI exactly. - tsconfig.json: add .archgate/ and scripts/ to include. - scripts/ensure-rules-dts.ts: new script wrapping the existing ensureRulesShim() helper (already used by `archgate check`/`init`). - package.json: `lint` now runs the script before oxlint. - ARCH-005's companion rule was the only .rules.ts file with a real import (node:path) and is the only one that still failed after the above — tsc --build resolves it fine, tsgolint doesn't (a parity gap, same category ARCH-026 documents). Replaced with two local string-based helpers instead, matching every other .rules.ts file's zero-import, ctx-sandboxed convention; fire-tested against a real missing-test-file case to confirm identical behavior. - ARCH-026 + agent memory updated with both root causes. Signed-off-by: Claude <noreply@anthropic.com> Signed-off-by: Rhuan Barreto <rhuan@barreto.work>
Contributor
Code Coverage
Full HTML report available in workflow artifacts. Per-directory breakdown
|
Investigated and confirmed dead configuration: it was load-bearing only until an early PR (well before this session) removed the `./rules` export and `defineRules()` helper it originally protected — consumers no longer type-check against archgate's own source. Every actual consumer touchpoint today is TypeScript-version-agnostic: the published npm shim (shims/npm/archgate.cjs, the only file in package.json's `files` array) never references TypeScript, and .rules.ts files load via Bun's own runtime transpilation (src/engine/loader.ts's `await import(...)`), never touching a consumer's installed typescript package. Since PR #86 it was only ever mechanically widened by Renovate with no functional purpose. Updated ARCH-026 (written earlier this session, before this was investigated) to stop describing the peer range as a still-existing, separate consumer-facing contract. Signed-off-by: Claude <noreply@anthropic.com> Signed-off-by: Rhuan Barreto <rhuan@barreto.work>
Addresses review feedback: the isEsTreeNode() type guard introduced by the type-aware lint fix pass only checked "is this a non-null object" — it never verified the `type: string` field every real ESTree node carries, so asEsTreeNode()/asEsTreeNodeArray() would silently narrow ANY object (e.g. a `loc` position object) as if it were a genuine AST node. Fixed across all 5 companion rule files that had it (ARCH-008, ARCH-012, ARCH-020, ARCH-022, ARCH-023). For the 3 files with a custom walk() (ARCH-008, ARCH-012, ARCH-022), split the old loose check into two: isPlainObject() (unchanged, still gates walk()'s tree descent so it keeps recursing into every object/array exactly as before) and a now-strict isEsTreeNode() (requires `type: string`, used everywhere a value is actually narrowed and treated as a node). ARCH-020/ARCH-023 have no custom walk — isEsTreeNode() there only ever narrowed, so it's fixed directly with no behavior-preservation concern. Fire-tested ARCH-012's async-action-error-boundary rule against two real probe violations (missing try-catch; an await escaping an existing try-catch, exercising containsDirectAwait()'s AST traversal specifically) to confirm the stricter check didn't regress detection. Signed-off-by: Claude <noreply@anthropic.com> Signed-off-by: Rhuan Barreto <rhuan@barreto.work>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
…l ADR Per direct feedback: .oxlintrc.json's own inline comments already document every decision ARCH-026 restated (rule selection, exclusions, version pinning), and the config file itself is the enforcement layer — a companion rule asserting "options.typeAware === true" can only ever confirm the config still says what it says, never catch anything the config wouldn't also just silently reflect. Removed the ADR and its companion rule, and the cross-references added to ARCH-014/ARCH-019 (their own substantive content — Boolean()/no-extra-boolean-cast, withPromptFix-adjacency — stays; only the pointer to the now-deleted ADR is removed). Signed-off-by: Claude <noreply@anthropic.com> Signed-off-by: Rhuan Barreto <rhuan@barreto.work>
rhuanbarreto
commented
Jul 26, 2026
rhuanbarreto
left a comment
rhuanbarreto
left a comment
Contributor
Author
There was a problem hiding this comment.
First batch. Those comments are valid across the entire repo.
…dback Address the first batch of PR review comments (valid across the repo): - lint/expect-expect.ts: replace the object+cast type guard with a real `isAstNode()` predicate using `in` narrowing, matching the .rules.ts pattern already fixed for the same brittleness. - adr/create.ts: validate --title/--domain at the Commander option level via argParser, so blank values fail fast instead of being sensed with string comparisons downstream. - adr/domain/remove.ts: derive existingPrefix from an `in`-guarded ternary instead of manually widening the Record's value type. - adr/import.ts: extract the version-presence check to a const before the ternary, per ARCH-014's Boolean()-as-sole-condition caveat. Signed-off-by: Rhuan Barreto <rhuan@barreto.work>
Sweep the codebase for the same two review-feedback patterns fixed in 24405bb and apply them wherever they recur, per "those comments are valid across the entire repo": - lint/no-bare-env-restore.ts had the identical object+cast type guard as lint/expect-expect.ts before its fix — replaced with the same isAstNode() predicate using `in` narrowing. - adr/list.ts (--domain) and adr/update.ts (--title/--domain/--files) had the same unvalidated-CLI-option gap as adr/create.ts — extracted the argParser into a shared src/helpers/cli-options.ts (rejectBlank), reused by all three commands plus create.ts itself, with a test file. - adr/sync.ts's findLocalAdr() had a dead `!== ""` check on a value that can never be empty by construction — dropped it. Left alone: credential-store.ts's Record<string, string | undefined> (legitimately spreads Bun.env, different case), several generic isRecord/isJsonRecord/isAstNodeLike guards that are correctly typed as generic objects (not falsely narrower), the no-unsafe-type-assertion casts in ast-support.ts/runner.ts/binary-upgrade.ts (genuinely irreducible, already justified inline), and check/review-context's internal --adr/--base/--domain engine plumbing (shared with non-CLI callers and tests, so a Commander-level fix wouldn't remove the internal checks the way it did for the adr subcommand family). Signed-off-by: Rhuan Barreto <rhuan@barreto.work>
rhuanbarreto
commented
Jul 26, 2026
rhuanbarreto
commented
Jul 26, 2026
…use zod for AST guards Third round of review feedback (--base/--domain/--adr option validation, unbounded ancestor loops, brittle AST-shape checks, non-idiomatic test matcher): - check.ts's --base/--adr and review-context.ts's --base/--domain now validate via the shared rejectBlank argParser (ARCH-008), same as the adr subcommand family. Lets context.ts's buildReviewContext() and loader.ts's loadRuleAdrs() drop their now-redundant `!== ""` checks on the resolved values. - upgrade.ts's findPackageRoot() and paths.ts's findProjectRoot() had identical unbounded `for (;;)` ancestor walks; both now cap at MAX_ANCESTOR_DEPTH (1000) as a hard stop against a pathological dirname() result, on top of the existing self-referential-root check. - upgrade.ts's install-method match logic replaced a single compound `c !== null && c.binDir !== null && c.binDir !== "" && ...` predicate with two explicit filter() type predicates before find() — the literal `filter(Boolean)` suggested doesn't narrow away null under strict TS, so explicit predicates were needed instead. - ast-support.ts's isAstNodeLike()/isPlainObject() now validate via zod schemas (z.record/z.union, matching the existing PlainObjectSchema pattern in formats/adr.ts) instead of hand-rolled typeof checks. - tests/integration/import.test.ts's expectKeys() helper now asserts via Bun's toContainKey matcher (subset semantics, matching the original `k in v` check) instead of a hand-rolled throw — not toContainAllKeys, which is an exact-match matcher and would have broken the multi-key-object call sites. Signed-off-by: Rhuan Barreto <rhuan@barreto.work>
…into claude/warm-watching-hollerith
…ing at the flagged spots
Prompted by "did you do a workflow to verify on the rest of the repo as
well?" — round 3 only touched the exact files named in review comments
plus two structural siblings noticed in passing. A real repo-wide grep
for the same patterns turned up more:
- .archgate/lint/oxlint.ts's isAstNode() had the identical `typeof ===
"object"` + `as {type?:unknown}` cast shape as lint/expect-expect.ts
and lint/no-bare-env-restore.ts before their earlier fixes (missed
because that sweep only checked lint/ at the repo root, not
.archgate/lint/) — same `"type" in value` narrowing fix applied.
- src/engine/loader.ts's isRecord() and src/helpers/init-project.ts's
isJsonRecord() had the same generic-object shape ast-support.ts's
isAstNodeLike()/isPlainObject() were converted to zod for — both now
validate via the same z.record(z.string(), z.unknown()) schema.
- Also DRY'd out along the way, per direct feedback ("do you actually
need to repeat it all? DRY it!"): a JSON-narrowing test helper
(isRecord/expectKeys, or a hasKeys boolean variant) was copy-pasted
with minor variations across 7 test files. Consolidated into one
canonical isRecord/expectKeys/expectArray in tests/test-utils.ts;
cli-harness.ts now re-exports expectKeys/expectArray from there
instead of defining its own, and every call site across the 7 files
was rewritten to the shared version (same assertions, same matcher
choice as the earlier toContainKey fix).
Signed-off-by: Rhuan Barreto <rhuan@barreto.work>
includeServerName defaults to true in @sentry/node-core, so every event sent os.hostname() as server_name regardless of the sendDefaultPii/ dataCollection settings already in place — Sentry's own option docs flag this as the CLI-app PII case. The prior comment claimed hostnames were already excluded by default, which was incorrect. Signed-off-by: Rhuan Barreto <rhuan.barreto@gmail.com> Signed-off-by: Rhuan Barreto <rhuan@barreto.work>
rhuanbarreto
commented
Jul 27, 2026
This was referenced Jul 27, 2026
…r typing - yaml-utils.ts / plugin-install.ts: isYamlValue() and isCursorHookArray() were hand-written recursive/shape guards; replace with zod schemas (z.lazy() for the recursive YamlValue case) mirroring the established PlainObjectSchema pattern used elsewhere in this PR. - binary-upgrade.ts: the previous double-cast-through-unknown for the stream reader was based on an incorrect diagnosis (claimed lib.dom was missing Bun's readMany augmentation). The real cause: this repo's tsconfig excludes the DOM lib, so Response.body resolves through undici-types to node:stream/web's own ReadableStream<any>. Fixed by importing ReadableStreamDefaultReader directly from node:stream/web and narrowing with a single, still-necessary justified cast. Addresses PR review feedback on #534. Signed-off-by: Rhuan Barreto <rhuan.barreto@gmail.com> Signed-off-by: Rhuan Barreto <rhuan@barreto.work>
Contributor
Author
|
@coderabbitai approve |
rhuanbarreto
enabled auto-merge (squash)
July 27, 2026 12:28
✅ Action performedComments resolved and changes approved. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Supersedes #529 (Renovate's TypeScript v7 peer-range bump). That PR alone widened
peerDependencies.typescriptbut did nothing else — TypeScript was never pinned as an explicit devDependency, so nothing would actually start using TS7. This PR does the real migration:typescript(7.0.2) and addsoxlint-tsgolint(7.0.2001) as explicit, exact-pinned devDependencies — previously TypeScript resolved only transitively from the peer range.options.typeAware: true) with 58 of the 59 currently-implementedtypescript/*type-aware rules at"error".typescript/prefer-readonly-parameter-typesis excluded (not in typescript-eslint's own recommended/strict presets; alone accounted for ~930 of ~3546 trial findings).require-await(both forms) is disabled — it directly contradictstypescript/promise-function-asyncfor the common "async mock trivially resolving a value" shape.typescript/no-deprecatedfor the first time — it was already configured in.oxlintrc.jsonbut silently inert since type-aware linting was never available before TS7.src/,tests/,lint/, and.archgate/adrs/*.rules.ts: preciseMock<T>typing forbun:testspies (replacing untypedReturnType<typeof spyOn>), real zod-schema narrowing in place of uncheckedJSON.parsecasts, explicit boolean coercion for nullable env/string checks per ARCH-014, async/await correctness, and narrow justified suppressions only for deliberately-fake test fixtures or genuine tsgolint/tsc parity gaps (documented inline).shims/(untyped distribution CJS) anddocs/(separate sub-project, deps not installed at root) out of type-aware analysis via.oxlintrc.jsonoverrides/ignorePatterns.Test plan
bun run lint— clean, zero findingsbun run typecheck— cleanbun run format:check— clean (repo-tracked files)bun run test— 1812 pass / 22 skip / 0 fail across 1834 testsbun run check— 50/50 ADR rules pass, including new ARCH-026 companion rulebun run knip— cleanbun run build:check— cleanorigin/main(no divergence)