mirror of
https://github.com/stablyai/orca.git
synced 2026-09-22 00:02:31 +00:00
Flip `anti-slop/no-shape-in-symbol-names` from "off" to "error" and clear
every violation under src, config, tests and mobile.
What the rule bans
------------------
The case-insensitive substring "shape" in any JS/TS identifier: variables,
functions, parameters, types, type parameters, class members, private names,
object-literal keys and JSX identifiers. The one exemption is a statically
accessed member read owned by another value (`zodObject.shape` is fine), so
third-party APIs stay readable without a suppression.
"Shape" names a value's structure rather than its domain role. `UserShape`,
`validateArgShape` and `errorShape` all tell you the symbol is "an object
with some fields" -- which is already what a type says -- while saying
nothing about what the value is for or who owns it. The rule forces the
name to carry the domain instead.
Violations fixed
----------------
689 violations across 109 files at baseline (verified by re-running the
audit against the pre-change tree with the rule set to "error").
Fix pattern
-----------
Rename for the domain role, not the structure:
-type FieldShape = 'list' | 'map' | 'whole'
-const FIELD_SHAPES = { ... } satisfies Record<keyof Observation, FieldShape>
+type FieldEncoding = 'list' | 'map' | 'whole'
+const FIELD_ENCODINGS = { ... } satisfies Record<keyof Observation, FieldEncoding>
-function assertGitPushTargetShape(target: unknown): void
+function assertValidGitPushTarget(target: unknown): void
-function describeReadDirPathShape(p: string): ReadDirPathKind
+function classifyReadDirPath(p: string): ReadDirPathKind
Predicates became statements about the value (`isDeltaShapedProviderFrameKind`
-> `isDeltaProviderFrameKind`, `isDeleteShapedDiscardEntry` ->
`discardDeletesEntryFile`, `isSkillsCliAgentKeyShaped` ->
`isUsableSkillsCliAgentKey`). Type aliases dropped the suffix where the
remaining name was already unambiguous (`GhGraphqlErrorShape` ->
`GhGraphqlError`).
No wire-visible name was renamed: no IPC or RPC channel, stream opcode,
request/response param, persisted field, or i18n key. The `--shape=symlink|copy`
CLI flag read by .github/workflows/skill-update-roundtrip.yml is unchanged --
only the local variable holding it was renamed.
Exemptions
----------
They are file-scoped entries in config/oxlint-anti-slop.json, not inline
`oxlint-disable` comments. An inline directive naming an anti-slop rule reads
back as an UNUSED directive under the root lint scan, which does not load this
plugin -- the changed-code quality gate counts that warning, so the comment form
cannot be used for a rule that lives only in this config.
* src/renderer/src/components/browser-pane/annotate/**:
in the screenshot annotator a "shape" is the drawn geometry -- pen, arrow,
rect, ellipse, highlight. That is a genuine domain noun, and it pervades
every symbol in the module.
* repo-icon.tsx, repo-header-project-actions.tsx, mobile MobileRepoIcon.tsx:
lucide exports the icon component as `Shapes`. The name is theirs, and the
matching REPO_LUCIDE_ICONS key is the persisted icon name shared with the
desktop picker -- renaming it would orphan saved repo icons.
* src/shared/onboarding-state-types.ts, src/shared/constants.ts:
`shapedSidebar` is a persisted onboarding-checklist field and a telemetry
enum member; renaming it would orphan saved state.
* src/shared/rpc-contract/rpc-send-params.ts: matching zod's own literal `shape`
property is what selects the ZodObject branch of the conditional type.
No exemption was added merely to avoid a rename. Eight symbols initially
suppressed as "a cross-module refactor outside this change" were proven to have
zero non-TypeScript references repo-wide and renamed instead.
Zod's `ZodRawShape` needed no exemption at all: `Readonly<Record<string,
z.ZodType>>` is its definition, so repo-update-params.ts and
ui-update-value-tolerance-params.ts spell it out instead. Likewise
telemetry-event-classification.ts now reads `.shape` through an `in` narrowing,
which also retires two pre-existing type assertions; three more assertions the
rename had dragged onto changed lines (two `JSON.parse` sites, one node:sqlite
row read) became annotations and an explicit row mapping.
Verified
--------
* Audit reports zero violations; confirmed the rule genuinely fires by
planting a probe violation.
* node config/scripts/run-typecheck-projects-in-parallel.mjs exits 0.
* Vitest over src/shared, src/main/github/project-view, the annotate module,
the repo-icon components and the Chromium SameSite electron spec: all green.
* All 66 removed "shape" identifiers grepped repo-wide across every file type;
none survive.
* node config/scripts/generate-rpc-params-catalog.mjs --check exits 0.
* node --check on every changed .mjs; oxfmt clean on all changed files.
* `pnpm run check:code-quality:changed` reports 0 findings.
Not machine-verified: the 3 mobile/ files (its Vitest run cannot resolve
`expo/tsconfig.base.json` in this worktree), and the WSL- and Playwright-gated
specs. All are rename- or comment-only hunks, read in full.
208 lines
8.0 KiB
TypeScript
208 lines
8.0 KiB
TypeScript
import { execFileSync } from 'node:child_process'
|
|
import { mkdirSync, mkdtempSync, rmSync, writeFileSync } from 'node:fs'
|
|
import { tmpdir } from 'node:os'
|
|
import * as path from 'node:path'
|
|
import { afterEach, beforeEach, describe, expect, it } from 'vitest'
|
|
import { RelayContext } from './context'
|
|
import { GitHandler } from './git-handler'
|
|
import {
|
|
createMockDispatcher,
|
|
gitCommit,
|
|
gitInit,
|
|
type MockDispatcher,
|
|
type RelayDispatcher
|
|
} from './git-handler-test-setup'
|
|
|
|
// Why this file exists: the pinned route replaced the legacy route's own
|
|
// `diff --name-status -M -C` rediscovery with values the caller already holds.
|
|
// These tests drive the real product contract against real Git and assert the
|
|
// two routes agree entry-for-entry, so "SSH still renders what it used to" is
|
|
// evidence rather than an argument about call sites.
|
|
|
|
type BranchCompareResult = {
|
|
summary: { mergeBase: string; headOid: string; status: string; changedFiles: number }
|
|
entries: { path: string; oldPath?: string; status: string }[]
|
|
}
|
|
|
|
type DiffEntry = Record<string, unknown>
|
|
|
|
function git(repoPath: string, args: string[]): string {
|
|
return execFileSync('git', args, { cwd: repoPath, encoding: 'utf8' })
|
|
}
|
|
|
|
/**
|
|
* Builds one repo whose base..head range exercises every shape the review
|
|
* panel can hand to a single-file branch diff.
|
|
*/
|
|
function buildScenarioRepo(): { repoPath: string; baseOid: string } {
|
|
const repoPath = mkdtempSync(path.join(tmpdir(), 'relay-branch-diff-equivalence-'))
|
|
gitInit(repoPath)
|
|
|
|
const write = (relativePath: string, contents: string | Buffer): void => {
|
|
const target = path.join(repoPath, relativePath)
|
|
mkdirSync(path.dirname(target), { recursive: true })
|
|
writeFileSync(target, contents)
|
|
}
|
|
|
|
write('modified.txt', 'before\n')
|
|
write('deleted.txt', 'doomed\n')
|
|
write('renamed-from.txt', 'stable rename payload\n')
|
|
write('renamed-and-edited-from.txt', 'rename plus edit, line one\nline two\nline three\n')
|
|
write('copied-source.txt', 'copy me, line one\nline two\nline three\nline four\n')
|
|
write('nested/deep/inner.txt', 'nested before\n')
|
|
write('spaced name.txt', 'spaced before\n')
|
|
write('ünïcode-ページ.txt', 'unicode before\n')
|
|
write('binary-modified.bin', Buffer.from([0, 1, 2, 3, 0, 255]))
|
|
write('binary-deleted.bin', Buffer.from([9, 8, 7, 0]))
|
|
write('emptied.txt', 'about to be emptied\n')
|
|
write('crlf.txt', 'crlf before\r\nsecond line\r\n')
|
|
write('mode-changed.sh', '#!/bin/sh\necho hi\n')
|
|
gitCommit(repoPath, 'base')
|
|
const baseOid = git(repoPath, ['rev-parse', 'HEAD']).trim()
|
|
|
|
write('modified.txt', 'after\n')
|
|
rmSync(path.join(repoPath, 'deleted.txt'))
|
|
rmSync(path.join(repoPath, 'binary-deleted.bin'))
|
|
git(repoPath, ['mv', 'renamed-from.txt', 'renamed-to.txt'])
|
|
git(repoPath, ['mv', 'renamed-and-edited-from.txt', 'renamed-and-edited-to.txt'])
|
|
write('renamed-and-edited-to.txt', 'rename plus edit, line one\nline two CHANGED\nline three\n')
|
|
write('copied-target.txt', 'copy me, line one\nline two\nline three\nline four\n')
|
|
write('added.txt', 'brand new\n')
|
|
write('binary-added.bin', Buffer.from([4, 4, 0, 4]))
|
|
write('binary-modified.bin', Buffer.from([0, 1, 2, 3, 0, 254]))
|
|
write('nested/deep/inner.txt', 'nested after\n')
|
|
write('spaced name.txt', 'spaced after\n')
|
|
write('ünïcode-ページ.txt', 'unicode after\n')
|
|
write('emptied.txt', '')
|
|
write('crlf.txt', 'crlf after\r\nsecond line\r\n')
|
|
git(repoPath, ['update-index', '--chmod=+x', 'mode-changed.sh'])
|
|
gitCommit(repoPath, 'head')
|
|
|
|
return { repoPath, baseOid }
|
|
}
|
|
|
|
describe('pinned and legacy branch diff equivalence against real Git', () => {
|
|
let dispatcher: MockDispatcher
|
|
let handler: GitHandler
|
|
let repoPath = ''
|
|
let baseOid = ''
|
|
|
|
beforeEach(() => {
|
|
dispatcher = createMockDispatcher()
|
|
handler = new GitHandler(dispatcher as unknown as RelayDispatcher, new RelayContext())
|
|
const built = buildScenarioRepo()
|
|
repoPath = built.repoPath
|
|
baseOid = built.baseOid
|
|
})
|
|
|
|
afterEach(() => {
|
|
handler.dispose()
|
|
if (repoPath) {
|
|
rmSync(repoPath, { recursive: true, force: true })
|
|
}
|
|
})
|
|
|
|
async function branchCompare(): Promise<BranchCompareResult> {
|
|
return (await dispatcher.callRequest('git.branchCompare', {
|
|
worktreePath: repoPath,
|
|
baseRef: baseOid
|
|
})) as BranchCompareResult
|
|
}
|
|
|
|
async function branchDiff(params: Record<string, unknown>): Promise<DiffEntry[]> {
|
|
return (await dispatcher.callRequest('git.branchDiff', {
|
|
worktreePath: repoPath,
|
|
baseRef: baseOid,
|
|
includePatch: true,
|
|
...params
|
|
})) as DiffEntry[]
|
|
}
|
|
|
|
it('produces identical results for every changed file the review panel can open', async () => {
|
|
const compare = await branchCompare()
|
|
expect(compare.summary.status).toBe('ready')
|
|
// Guard the guard: a truncated scenario set would make this test vacuously pass.
|
|
expect(compare.entries.length).toBeGreaterThanOrEqual(14)
|
|
|
|
const divergences: string[] = []
|
|
for (const entry of compare.entries) {
|
|
// Exactly what the renderer sends: paths from the compare entry list,
|
|
// OIDs from the compare summary that produced that same list.
|
|
const callerParams = { filePath: entry.path, oldPath: entry.oldPath }
|
|
const legacy = await branchDiff(callerParams)
|
|
const pinned = await branchDiff({
|
|
...callerParams,
|
|
baseRef: compare.summary.mergeBase,
|
|
headOid: compare.summary.headOid
|
|
})
|
|
|
|
if (JSON.stringify(legacy) !== JSON.stringify(pinned)) {
|
|
divergences.push(
|
|
`${entry.status} ${entry.path}${entry.oldPath ? ` (from ${entry.oldPath})` : ''}\n` +
|
|
` legacy: ${JSON.stringify(legacy)}\n pinned: ${JSON.stringify(pinned)}`
|
|
)
|
|
}
|
|
}
|
|
|
|
expect(divergences.join('\n')).toBe('')
|
|
})
|
|
|
|
it('agrees on content for renames, additions, deletions and binaries specifically', async () => {
|
|
const compare = await branchCompare()
|
|
const byPath = new Map(compare.entries.map((entry) => [entry.path, entry]))
|
|
|
|
// Why assert content and not just equality: two identically-empty results
|
|
// would satisfy the equivalence test above while rendering nothing.
|
|
const rename = byPath.get('renamed-and-edited-to.txt')
|
|
expect(rename?.oldPath).toBe('renamed-and-edited-from.txt')
|
|
const [renamePinned] = await branchDiff({
|
|
baseRef: compare.summary.mergeBase,
|
|
headOid: compare.summary.headOid,
|
|
filePath: rename!.path,
|
|
oldPath: rename!.oldPath
|
|
})
|
|
expect(renamePinned).toMatchObject({
|
|
originalContent: 'rename plus edit, line one\nline two\nline three\n',
|
|
modifiedContent: 'rename plus edit, line one\nline two CHANGED\nline three\n'
|
|
})
|
|
|
|
const [addedPinned] = await branchDiff({
|
|
baseRef: compare.summary.mergeBase,
|
|
headOid: compare.summary.headOid,
|
|
filePath: 'added.txt'
|
|
})
|
|
expect(addedPinned).toMatchObject({ originalContent: '', modifiedContent: 'brand new\n' })
|
|
|
|
const [deletedPinned] = await branchDiff({
|
|
baseRef: compare.summary.mergeBase,
|
|
headOid: compare.summary.headOid,
|
|
filePath: 'deleted.txt'
|
|
})
|
|
expect(deletedPinned).toMatchObject({ originalContent: 'doomed\n', modifiedContent: '' })
|
|
|
|
const [binaryPinned] = await branchDiff({
|
|
baseRef: compare.summary.mergeBase,
|
|
headOid: compare.summary.headOid,
|
|
filePath: 'binary-modified.bin'
|
|
})
|
|
expect(binaryPinned).toMatchObject({ kind: 'binary' })
|
|
})
|
|
|
|
it('holds the pinned revision when HEAD moves mid-review, where legacy drifts', async () => {
|
|
const compare = await branchCompare()
|
|
writeFileSync(path.join(repoPath, 'modified.txt'), 'drifted after the snapshot\n')
|
|
gitCommit(repoPath, 'drift')
|
|
|
|
const pinned = await branchDiff({
|
|
baseRef: compare.summary.mergeBase,
|
|
headOid: compare.summary.headOid,
|
|
filePath: 'modified.txt'
|
|
})
|
|
const legacy = await branchDiff({ filePath: 'modified.txt' })
|
|
|
|
expect(pinned[0]).toMatchObject({ modifiedContent: 'after\n' })
|
|
// The divergence is the fix: legacy silently re-resolves live HEAD.
|
|
expect(legacy[0]).toMatchObject({ modifiedContent: 'drifted after the snapshot\n' })
|
|
})
|
|
})
|