Repository navigation
chore: codebase-review cleanups — serial I/O, keychain triple-read, type-registration drift, dead code #35
Copy link
Copy link
Description
Verified cleanup/efficiency/altitude findings from the 2026-07-08 whole-codebase review that don't overlap #17. None are release blockers; batched so they don't get lost. Ordered: efficiency (user-visible latency) → altitude (future breakage) → dead code.
Efficiency (API round-trips are ~100ms each; these are user-visible)
dynamicField.foldruns 2N serial round-trips (src/engine/synthetic.ts:82-103): ruleset + status awaited sequentially per group in a sequential loop.mapConcurrent(used by build.ts at concurrency 8) +Promise.allper pair → ~8× faster plan/apply with many dynamic groups.buildPlanandbuildPermissionPlanawaited back-to-back though independent (commands/apply.ts:99-100,commands/plan.ts:34-35):Promise.allhides the slow instance-wide/permissions/group_rolefetch behind the concurrent resource fetches.- Permission writes fully serial (
permissions/apply.ts:24-34): one PUT/DELETE per tuple; first rollout of a permissions blueprint (50+ grants) ≈ 5s. Modest concurrency (4–8) per item is safe — tuples are independent rows. - Keychain read 3× per run:
resolveConfig()(command) +resolveConfig()+readToken()(authedSession) each spawnsecurity find-generic-passwordfor the same entry (commands/plan.ts:25,src/api/session.ts:15-16). Memoize per process or have authedSession return its resolved config. Up to three keychain-unlock prompts on a locked keychain.
Altitude / future breakage
--refreshhardcodes thedynamicfield + demote sentinel in the command layer (commands/apply.ts:52-55): duplicates knowledge owned bysynthetic.ts(:96-97, :113).SyntheticFieldhas no post-apply hook — add one (postApply?(client, state, item, changes)) and move refresh behind it; renaming the field today silently breaks--refresh(find() matches nothing).- Type registration is scattered, and TYPE_TIER carries phantom types: adding a resource type requires
registry.ts+graph.tsTYPE_TIER (computePlan throws otherwise,plan.ts:85-89) +config/context.tsDSL entry — despiteregistry.ts:7claiming "adding a type = adding an entry here".TYPE_TIERlistsgroup-statusetc. that exist nowhere else. Consolidate: registry entry owns tier + DSL name (which also givesconfigSnippetthe correct function name — see the groupRole-collision bug) and derive TYPE_TIER/ConfigContext from it. - Engine trusts raw arrays — dup-key silently last-wins: dup-key/reference validation lives only in the DSL path (
context.ts:146-167, :200);computePlanbuildsdesiredByKeythat silently collapses duplicates while the plan loop emits both (plan.ts:92-97). All CLI paths go throughevaluateConfigtoday, but the first programmatic caller (import command, test harness) gets state-corrupting behavior. Cheap fix:computePlanthrows on duplicate keys. - Envelope/bootstrap duplication in ctClient (
ctClient.ts:50-67, :110-125):refreshCsrfTokencan bethis.get('/csrftoken')(it runs with a cookie; GET skips the CSRF branch).authenticategenuinely can't reuserequest()(cookie bootstrap) — but both currently assume.dataexists whilerequest()tolerates its absence; align the unwrap.
Dead code / micro
- Unreachable host check in
commands/plan.ts:29-31—loadState(path, config.host)two lines above already throws on mismatch (state.ts:70-73), with a different message. Delete. - Opt-in predicate encoded 3× per fold (
hierarchy.ts:79,83,93;synthetic.ts:79,83,105): the optedIn-Set inverted-iteration pattern; a single pass overdesiredwith astate.resources[d.key]lookup is behavior-identical (preserve the managed-type guard) and leaves one copy of the predicate. - No-op sort in
permissions/grants.ts:23-24(dataIdis[]or single-element;tupleKeyre-sorts anyway) — implies a multi-id invariant that doesn't exist; drop it. loadCatalog()naming (permissions/catalog.ts:16-18): wraps a build-time import constant, "loads" nothing; export the constant (callers already defensively snapshot it).
Acceptance
- Items 1–4: plan/apply wall-clock measurably improves on a config with ≥10 dynamic groups + grants (before/after timing note in the PR).
- Items 5–7: registry-driven tier/DSL-name; postApply hook; computePlan dup-key throw — each with a test.
- Items 9–12: deleted/simplified with tests still green.
Reactions are currently unavailable
Metadata
Metadata
Assignees
Labels
No labels
Activity
2000game commentedon Jul 9, 2026
Addendum from the PR #18 three-agent review — minor follow-ups that belong in this cleanup batch:
groupRole/groupTypeRoleblock leaves its applied grants live forever, andct destroynever touches permissions. Terraform-contract violation; needs state-tracked permission domains so removal plans deletions. (This is the largest item here — promote to its own issue when picked up.)permissions/apply.tsbare loop): a mid-loop failure throws a raw stack after resources were already applied, without the resumable "Stopped at …" messagingexecutePlanprovides. Re-running recovers, but the UX contradicts the resource path.synthetic.tsapply path): a pure status flip (active→inactive) also rewrites the byte-identical ruleset; wasteful and may trigger server-side recalculation.ct plan --jsonoutput shape changed from the bare plan object to{ plan, permissions }— breaking for script consumers; note in changelog/README when a changelog exists.Merge pull request #41 from eqrm/chore/codebase-cleanups-35