refactor: nuclear-review pass — hook-command unification, installer dedup, Codex-bridge hardening#61
Merged
Merged
Conversation
…ing, exec diff, output sanitization - --force bypasses a sticky rate-limited/no-access verdict (Codex emits 'quota exceeded' on auth mismatch too, so a misfire shouldn't disable the bridge globally for 5h with no escape) - skip the per-call 'codex login status' spawn when a fresh 'available' verdict is <60s old (AVAILABLE_TTL_MS) - detect timeouts explicitly and surface partial output + a split/raise hint instead of an opaque 'exit N' - exec appends git status/diff --stat so the caller always sees what Codex changed - sanitizeOutput() (ANSI + secret redaction) now runs over returned output, not just the cached stderr detail
- writeState now writes tmp+rename (was a bare writeFile mislabelled 'Atomic-ish'); a crash mid-write can no longer corrupt counter/queue state - CounterState rehydration goes through normalizeCounterState(unknown), removing the inline re-defaulting and the 'as number | undefined' cast; always-present fields are now required on the interface
analyzeCommands(label, files) -> AuditModel (pure aggregation) and renderAudit(model) -> string (pure formatting). New report sections become data-only; output is byte-identical for the same input.
lint-skills and lint-knowledge shared ~80% scaffolding (dir walk, missing frontmatter, yaml errors, name mismatch). lintFrontmatterCore() owns the shared path; each linter keeps only its domain rules. Rule keys and messages unchanged.
Single src/lib/hook-command.ts (parseHookCommand / isManagedHookCommand / MANAGED_HOOK_CMD / iterCommandHooks) replaces three divergent definitions in settings-merge, light-profile, and audit-hooks. The trusted regex is unified to (scripts|hooks) only — drops lib/ from audit's TRUSTED_BUN_CC (no shipped hook invokes lib/; tightening the trusted surface is the safe direction). Hook-block traversal goes through HooksBlock-schema-driven iterCommandHooks instead of three hand-rolled walks. Also: delete the teamRaw alias; model audit findings as a HookFinding|SchemaFinding discriminated union (no more sentinel-string matching); validate the fingerprint record through zod in readFingerprint. canonicalize()/hash unchanged so existing fingerprints stay valid. Security: lib/ commands now classify as untrusted (1 flipped test); fail-open preserved; [ \t] arg-separation kept (not widened to \s).
…LLS + lint guard - buildInstallPlan(sourceDir, profile) is the single source of truth for disk-touching install/prune steps; install and light-incompatible removal derive from it instead of re-walking PROFILE_MANIFEST - main() splits into runMigrateOnly / runFullInstall behind a thin dispatcher; full-install phase order preserved exactly (clean before copy, fingerprint after settings write, manifest for tamper defense) - removed the redundant in-installer skill-prune loop — BUT it was masking a real gap: 7 current skills (codex, freeze, plan-ceo-review, proof-of-work, retro, review-batch, strategist) were missing from the managed list, so cleanOldConfig didn't prune them on a full→light switch (caught by install-e2e) - split managed-skills into ACTIVE_SKILLS (35, complete) + TOMBSTONE_SKILLS; MANAGED_SKILLS = union, consumers unchanged - lint:skills now asserts ACTIVE_SKILLS matches skills/ on disk (opt-in, repo-run only) so this drift becomes a lint error, not silent install drift Deferred: MCP-preservation boundary move (Change 4) — making the merger pure requires changing its API + rewriting MCP-preservation tests; not a pure behavior-preserving refactor. Tracked separately.
settings-merge.ts is now a pure JSON merge (renamed mergeSettings) that takes a pre-resolved mcpServers value and no longer imports mcp.ts. The MCP-preservation semantics move to mcp.ts as resolveMcpServers(user, team); the mergeSettingsWithMcpPreservation wrapper now lives there and composes resolveMcpServers + mergeSettings. Observable behavior is unchanged — tests only move their import source; every preservation assertion (which servers survive, precedence, _status) is identical.
From the security-reviewer + Codex gate on this branch: - readSrcManifest rejects manifest keys with ..\/absolute paths and non-string hash values (the manifest is the Shai-Hulud-targeted artifact; keys are join()'d under ~/.claude/src and read) - strip C0/C1 control chars from fingerprint + manifest installedAt before it's echoed into the warning banner (ANSI-injection guard) - sanitizeOutput also redacts *_API_KEY|TOKEN|SECRET= assignments and Bearer: (colon, no space) forms - document why classifyCompound's all-parts-managed check must never relax to some() (|| branches still run) Adds 10 tests covering the new redaction forms + manifest validation + control strip.
Three files said 34 skills; there are 35 (freeze uncounted). lint:skills now enforces ACTIVE_SKILLS == skills/ so the count can't silently drift again.
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.
What this does
A whole-codebase cleanup pass from
/nuclear-reviewplus a set of Codex-bridge fixes. Three things a user/maintainer would actually feel:--forceoverrides it, repeatedavailablechecks don't re-spawncodex login statusevery call, timeouts say what happened instead of an opaque exit code, andexecshows you the diff it made.~/.claude/srchook command" (which had already drifted apart on whetherlib/counts) are now one module, tightened to the safe(scripts|hooks)set.lint:skillsnow fails if the list ever drifts fromskills/again.No behavior change for a normal install — the refactors are behavior-preserving and the full suite (594 tests) stays green.
Summary
codex.ts,codex-run.ts):--forceescape past stickyrate-limited/no-access; 60s positive-TTL skips the per-call login probe; explicit timeout reporting w/ partial output;execappendsgit status/diff --stat;sanitizeOutputstrips ANSI + redactssk-/Bearer/Authorization/*_API_KEY|TOKEN|SECRET=on all returned output.src/lib/hook-command.ts(parseHookCommand/isManagedHookCommand/MANAGED_HOOK_CMD/iterCommandHooks) replaces the three classifiers insettings-merge/light-profile/audit-hooks; trusted regex tightened(scripts|hooks|lib)→(scripts|hooks); traversal goes through theHooksBlockschema.audit-hooksfindings are now aHookFinding|SchemaFindingunion.hooks-fingerprint.ts): zod-validatedreadFingerprint;readSrcManifestrejects../absolute keys + non-string values; control chars stripped frominstalledAtbefore the banner echoes it.setup.ts):buildInstallPlansingle source of truth;main()split intorunMigrateOnly/runFullInstall; redundant skill-prune loop removed;managed-skills.tssplit intoACTIVE_SKILLS(now complete) +TOMBSTONE_SKILLS;lint:skillsassertsACTIVE_SKILLS == skills/.mcp.tsresolveMcpServers(settings-merge.tsno longer importsmcp.ts).claude-audit.tssplit into model+render; shared frontmatter-lint core extracted; atomicwriteState;normalizeCounterState.Reviewed cross-model: an independent security-reviewer pass and a Codex review of the diff both approved it; their hardening findings are folded in (the
harden(security)commit).Test Plan
bun run typecheck— cleanbun test— 594 pass / 0 fail (+10 new: redaction forms, manifest path-traversal rejection, control-char strip)bun run lint— cleanbun run audit:hooks— 27 trusted, 0 stale/unknown/suspiciousbun run schemas:check— in syncinstall-e2efull→light switch — passes (the bug this caught)