chore: codebase-review cleanups — concurrency, keychain memoization, registry-owned tiers, postApply hook, dead code (#35) - #41
Merged
Conversation
…UT, dup-key guard (#35) - item 1: dynamicField.fold fetches each group's ruleset+status via mapConcurrent (concurrency 8) instead of 2N serial round-trips; per-group errors collected in input order so plan degradation stays deterministic. - item 10: fold + applyHierarchy iterate desired opt-ins once (state lookup) instead of build-a-Set-then-invert-over-state; one copy of the predicate, managed-type guard kept. - item 15: dynamicField.apply skips the byte-identical ruleset re-PUT on a pure status flip (deepEqual, now exported from plan.ts); still PUTs both when the ruleset changed. - item 7: computePlan throws on duplicate desired keys instead of silently last-wins. - item 5 (partial): SyntheticField.postApply hook + runPostApplyHooks driver; the dynamic refresh moves off the command layer onto the dynamic field.
…erm apply (#35) - item 2: buildPlan + buildPermissionPlan run via Promise.all in plan/apply — the slow instance-wide /permissions/<domainType> fetch hides behind the resource fetches. - item 3: permission writes fan out at concurrency 6 (independent rows); result counts collected in flattened op order, deterministic regardless of completion order. Preserves PR #39's per-tuple re-resolution against post-execute state. - item 14: a failed permission write is captured in `failed` instead of aborting the batch; apply prints a clean resumable summary (which tuples failed) and exits non-zero, mirroring executePlan's 'Stopped at ...' stance. - item 4: keychain read memoized per process with resetKeychainCache() (invalidated on store/clear) — one `security` spawn per run instead of up to three. - item 5/9 wiring: apply drives runPostApplyHooks for --refresh; plan drops the unreachable host check (loadState already throws on mismatch).
… drop phantoms (#35 item 6) Each RESOURCES entry now carries its `tier`; engine/graph.ts derives TYPE_TIER from the registry instead of a parallel hand-maintained table. Removes the phantom entries (group-status, group-hierarchy, permission, dynamic-group) that were never DesiredResource types. Export shape (Record<string, number>) unchanged so computePlan keeps working. Tests lock registry<->TYPE_TIER and registry<->ConfigContext in sync.
- item 8: refreshCsrfToken is now `this.get('/csrftoken')` (GET skips the CSRF branch, so no
recursion) and reuses request()'s guarded unwrap; authenticate aligns to the same tolerant
`.data ?? body` unwrap. Preserves assertMinVersion + guarded 2xx parsing.
- item 11: drop the no-op dataId sort in normalizeActual (dataId is [] or single-element;
tupleKey sorts defensively anyway).
- item 12: export the catalog as the CATALOG constant instead of a loadCatalog() wrapper that
loads nothing; update callers.
…pool invariant comment
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.
Implements #35 items 1–12 plus addendum items 14–15. Item 13 (permission revocation state-tracking) is deliberately left open as a feature; item 16 (changelog note) waits for a changelog to exist.
Efficiency (items 1–4)
mapConcurrent(8) — kept ruleset→status sequential per group (the status GET's 404/sentinel semantics depend on the ruleset GET's outcome).buildPlan+buildPermissionPlannow run underPromise.allin plan and apply.securityspawns per run) with exported cache reset, invalidated on store/clear.Altitude (items 5–7)
SyntheticField.postApplyhook:--refreshno longer hardcodes the dynamic field + demote sentinel in the command layer.tier;TYPE_TIERis derived, phantom types dropped; registry↔ConfigContext sync locked by test.computePlanthrows on duplicate desired keys instead of silently last-winning.Dead code / micro (items 8–12) + addendum
refreshCsrfTokenreusesthis.get;authenticateunwrap aligned withrequest().CATALOGexported as a constant.Rebased onto main after PR #40; the one overlap (
applyHierarchysingle-pass, done independently on both branches) resolved keeping the variant with the explicit managed-type guard.Verification: 272 passed / 4 skipped, typecheck + lint clean.
Closes #35.