mirror of
https://github.com/stablyai/orca.git
synced 2026-10-07 16:02:29 +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.
230 lines
7.3 KiB
TypeScript
230 lines
7.3 KiB
TypeScript
/**
|
|
* Git exec argument validation for the relay's git.exec handler.
|
|
*
|
|
* Why: oxlint max-lines requires files to stay under 300 lines.
|
|
* Extracted from git-handler-ops.ts to keep both files under the limit.
|
|
*/
|
|
|
|
import {
|
|
isSafeGitRemoteName,
|
|
isSafePushTargetRemoteUrl
|
|
} from '../shared/git-push-target-validation'
|
|
|
|
// Why: only read-only git subcommands are allowed via exec, except for the
|
|
// exact init/empty-commit shapes used by SSH Create Project after the parent
|
|
// directory has already been validated by main, and the exact fork-remote
|
|
// add/remove shapes used by SSH fork-PR worktrees.
|
|
const ALLOWED_GIT_SUBCOMMANDS = new Set([
|
|
'rev-parse',
|
|
'branch',
|
|
'log',
|
|
'show-ref',
|
|
'ls-remote',
|
|
'remote',
|
|
'symbolic-ref',
|
|
'merge-base',
|
|
'diff',
|
|
'ls-files',
|
|
'clone',
|
|
'init',
|
|
'commit',
|
|
'for-each-ref',
|
|
'check-ref-format',
|
|
'config'
|
|
])
|
|
const CONFIG_READ_ONLY_FLAGS = new Set(['--get', '--get-all', '--list', '--get-regexp', '-l'])
|
|
// Why: checking presence of a read-only flag is insufficient — a request could
|
|
// include both --list (passes the check) and --add (performs a write). Reject
|
|
// known write operations explicitly.
|
|
const CONFIG_WRITE_FLAGS = new Set([
|
|
'--add',
|
|
'--unset',
|
|
'--unset-all',
|
|
'--replace-all',
|
|
'--rename-section',
|
|
'--remove-section',
|
|
'--edit',
|
|
'-e',
|
|
// Why: --file redirects config reads to an arbitrary file, enabling path
|
|
// traversal (e.g. `--file /etc/passwd --list` leaks file contents).
|
|
'--file',
|
|
'-f',
|
|
'--global',
|
|
'--system'
|
|
])
|
|
const BRANCH_DESTRUCTIVE_FLAGS = new Set([
|
|
'-d',
|
|
'-D',
|
|
'--delete',
|
|
'-m',
|
|
'-M',
|
|
'--move',
|
|
'-c',
|
|
'-C',
|
|
'--copy'
|
|
])
|
|
|
|
// Why: these flags are dangerous across ALL subcommands — --output writes to
|
|
// arbitrary paths, --exec-path changes where git loads helpers from, --work-tree
|
|
// and --git-dir escape the validated worktree.
|
|
const GLOBAL_DENIED_FLAGS = new Set(['--output', '-o', '--exec-path', '--work-tree', '--git-dir'])
|
|
|
|
const REMOTE_WRITE_SUBCOMMANDS = new Set([
|
|
'add',
|
|
'remove',
|
|
'rm',
|
|
'rename',
|
|
'set-head',
|
|
'set-branches',
|
|
'set-url',
|
|
'prune',
|
|
'update'
|
|
])
|
|
const SYMBOLIC_REF_WRITE_FLAGS = new Set(['-d', '--delete', '-m'])
|
|
const DIFF_ALLOWED_FLAGS = new Set([
|
|
'--cached',
|
|
'--staged',
|
|
'--name-status',
|
|
'--patch',
|
|
'--minimal',
|
|
'--no-color',
|
|
'--no-ext-diff'
|
|
])
|
|
|
|
// Why: fork-PR worktrees on an SSH host must add the contributor's fork as a
|
|
// remote (and drop it again when the last worktree using it is removed). Allow
|
|
// only those two exact shapes, held to the same remote-name and URL rules the
|
|
// relay already enforces on every pushTarget-carrying RPC. Everything else --
|
|
// set-url, rename, prune, flags before the action -- stays blocked.
|
|
function isAllowedRemoteWriteInvocation(args: string[]): boolean {
|
|
if (args[1] === 'add') {
|
|
return args.length === 4 && isSafeGitRemoteName(args[2]) && isSafePushTargetRemoteUrl(args[3])
|
|
}
|
|
if (args[1] === 'remove') {
|
|
return args.length === 3 && isSafeGitRemoteName(args[2])
|
|
}
|
|
return false
|
|
}
|
|
|
|
function validateCloneArgs(args: string[]): void {
|
|
// Why: project-host setup needs remote clone, but git.exec must not become a
|
|
// general write surface. Permit only `git clone [--progress] -- <url> <dir>`.
|
|
const allowed = args[1] === '--progress' ? args.slice(2) : args.slice(1)
|
|
if (allowed.length !== 3 || allowed[0] !== '--') {
|
|
throw new Error('git clone via exec is restricted to clone [--progress] -- <url> <dir>')
|
|
}
|
|
const targetDir = allowed[2]
|
|
if (
|
|
!targetDir ||
|
|
targetDir === '.' ||
|
|
targetDir === '..' ||
|
|
targetDir.includes('/') ||
|
|
targetDir.includes('\\') ||
|
|
targetDir.includes('\0')
|
|
) {
|
|
throw new Error('git clone target directory must be a single safe path segment')
|
|
}
|
|
}
|
|
|
|
function validateInitArgs(args: string[]): void {
|
|
if (args.length !== 1) {
|
|
throw new Error('git init via exec is restricted to init with no arguments')
|
|
}
|
|
}
|
|
|
|
function validateCommitArgs(args: string[]): void {
|
|
if (args.length !== 4 || args[1] !== '--allow-empty' || args[2] !== '-m' || !args[3]) {
|
|
throw new Error('git commit via exec is restricted to commit --allow-empty -m <message>')
|
|
}
|
|
}
|
|
|
|
// Why: git accepts --flag=value compound syntax (e.g. --file=/etc/passwd),
|
|
// which bypasses exact-match Set.has() checks. This helper catches both forms.
|
|
function matchesDeniedFlag(arg: string, denySet: Set<string>): boolean {
|
|
if (denySet.has(arg)) {
|
|
return true
|
|
}
|
|
const eqIdx = arg.indexOf('=')
|
|
if (eqIdx > 0) {
|
|
return denySet.has(arg.slice(0, eqIdx))
|
|
}
|
|
return false
|
|
}
|
|
|
|
export function validateGitExecArgs(args: string[]): void {
|
|
// Why: git accepts `-c key=value` before the subcommand, which can override
|
|
// config and execute arbitrary commands (e.g. core.sshCommand). Reject any
|
|
// arguments before the subcommand that look like global git flags.
|
|
let subcommandIdx = 0
|
|
while (subcommandIdx < args.length && args[subcommandIdx].startsWith('-')) {
|
|
subcommandIdx++
|
|
}
|
|
if (subcommandIdx > 0) {
|
|
throw new Error('Global git flags before the subcommand are not allowed')
|
|
}
|
|
|
|
const subcommand = args[0]
|
|
if (!subcommand || !ALLOWED_GIT_SUBCOMMANDS.has(subcommand)) {
|
|
throw new Error(`git subcommand not allowed: ${subcommand ?? '(empty)'}`)
|
|
}
|
|
const restArgs = args.slice(1)
|
|
|
|
if (restArgs.some((a) => matchesDeniedFlag(a, GLOBAL_DENIED_FLAGS))) {
|
|
throw new Error('Dangerous git flags are not allowed via exec')
|
|
}
|
|
|
|
if (subcommand === 'config') {
|
|
if (!restArgs.some((a) => CONFIG_READ_ONLY_FLAGS.has(a))) {
|
|
throw new Error('git config is restricted to read-only operations (--get, --list, etc.)')
|
|
}
|
|
if (restArgs.some((a) => matchesDeniedFlag(a, CONFIG_WRITE_FLAGS))) {
|
|
throw new Error('git config write operations are not allowed via exec')
|
|
}
|
|
}
|
|
if (subcommand === 'init') {
|
|
validateInitArgs(args)
|
|
}
|
|
if (subcommand === 'commit') {
|
|
validateCommitArgs(args)
|
|
}
|
|
if (subcommand === 'branch') {
|
|
if (restArgs.some((a) => matchesDeniedFlag(a, BRANCH_DESTRUCTIVE_FLAGS))) {
|
|
throw new Error('Destructive git branch flags are not allowed via exec')
|
|
}
|
|
}
|
|
if (subcommand === 'remote') {
|
|
const remoteSubcmd = restArgs.find((a) => !a.startsWith('-'))
|
|
if (
|
|
remoteSubcmd &&
|
|
REMOTE_WRITE_SUBCOMMANDS.has(remoteSubcmd) &&
|
|
!isAllowedRemoteWriteInvocation(args)
|
|
) {
|
|
throw new Error('Destructive git remote operations are not allowed via exec')
|
|
}
|
|
}
|
|
if (subcommand === 'symbolic-ref') {
|
|
if (restArgs.some((a) => matchesDeniedFlag(a, SYMBOLIC_REF_WRITE_FLAGS))) {
|
|
throw new Error('git symbolic-ref write operations are not allowed via exec')
|
|
}
|
|
const positionalArgs = restArgs.filter((a) => !a.startsWith('-'))
|
|
if (positionalArgs.length >= 2) {
|
|
throw new Error('git symbolic-ref write operations are not allowed via exec')
|
|
}
|
|
}
|
|
if (subcommand === 'diff') {
|
|
// Why: SSH commit-message generation only needs read-only staged diffs.
|
|
// Keep this narrow so `git.exec` cannot become a general file reader via
|
|
// arbitrary revisions, pathspecs, or --no-index.
|
|
if (!restArgs.some((a) => a === '--cached' || a === '--staged')) {
|
|
throw new Error('git diff via exec is restricted to staged changes')
|
|
}
|
|
const unsupportedArg = restArgs.find((a) => !DIFF_ALLOWED_FLAGS.has(a))
|
|
if (unsupportedArg) {
|
|
throw new Error(`git diff flag not allowed via exec: ${unsupportedArg}`)
|
|
}
|
|
}
|
|
if (subcommand === 'clone') {
|
|
validateCloneArgs(args)
|
|
}
|
|
}
|