Skip to content

feat(hooks): HooksManager orchestration and HooksParser validation (3/7) - #674

Open
gemammercado wants to merge 1 commit into
mainfrom
hooks-pr3-manager
Open

feat(hooks): HooksManager orchestration and HooksParser validation (3/7)#674
gemammercado wants to merge 1 commit into
mainfrom
hooks-pr3-manager

Conversation

@gemammercado

Copy link
Copy Markdown
Contributor

PR 3 of 7 — hook orchestration and request parsing (stacked)

Third slice of the CloudFormation Hooks feature, following #655 (cache infra) and #662 (AWS service layer), both merged. This adds the layer between the LSP handlers and the AWS services. Handlers and LSP wiring come in a later slice, so nothing here is reachable from the server yet.

What's in it

HooksManager — orchestrates hook listing and detail lookup on top of CfnService:

  • Paginated listing. listHooks() starts a fresh page set; listHooks(true) appends the next page using the retained nextToken, deduplicating by type name. Requesting more when there is no nextToken is a no-op that returns what is already cached rather than silently refetching page one.
  • Detailed listing. listHooksDetailed() enriches each listed hook with its type configuration. Fetches are batched at a concurrency of 10 so a large hook count cannot fan out into hundreds of simultaneous API calls. A configuration fetch that fails degrades that hook to "not configured" instead of failing the whole listing.
  • Detail caching. describeHook() accepts either a type name or an ARN and caches the result under both identifiers, so looking a hook up one way and then the other does not trigger a second API call. Requesting neither identifier is rejected up front.
  • Rule content caching. getCachedRuleContent() reads Guard rule files through the TTL HookCache from feat(hooks): add TTL HookCache and persistent hook schema store #655, so repeated reads of the same S3 object do not re-download it.
  • Configuration parsing. parseHookConfiguration() reads the nested CloudFormationConfiguration.HookConfiguration wrapper and extracts failure mode, invocation status, target operations, and rule URI. Malformed JSON, a missing wrapper, or unexpected value types yield "not configured" rather than throwing.

HooksParser — zod schemas that validate and narrow the parameters of every hook LSP request before they reach a handler, so malformed client input produces a clear validation error instead of a downstream failure. Includes a Guard-hook payload check that reports each missing required key.

Testing

Unit tests for both files: pagination (including the no-more-pages case), deduplication, detail caching across both identifiers, the batching boundary with 25 hooks (asserting every hook resolves and peak concurrency stays within the limit), rule-content caching and loader error propagation, configuration parsing edge cases, and all parser schemas including malformed and missing input. Build, lint, and tests pass.

Stack

Base: main (requires #655 and #662, both merged). Next slice: local Guard hook preview.

@gemammercado
gemammercado requested a review from a team as a code owner August 31, 2026 14:12
Comment thread src/hooks/HooksManager.ts Fixed
@github-code-quality

github-code-quality Bot commented Aug 31, 2026

Copy link
Copy Markdown

Code Coverage Overview

Languages: TypeScript

TypeScript / code-coverage/vitest

The overall line coverage in commit 4a8b23b in the hooks-pr3-manager branch remains at 90%, unchanged from commit a1a3d82 in the main branch.

Show a line coverage summary of the most impacted files.
File main a1a3d82 hooks-pr3-manager 4a8b23b +/-
src/hooks/HooksManager.ts 0% 100% +100%
src/hooks/HooksParser.ts 0% 100% +100%

Updated September 01, 2026 22:06 UTC

Comment thread src/hooks/HooksParser.ts

@kddejong kddejong left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, well-scoped addition — strong input validation and sensible caching throughout. A few notes:

Suggested change before merge

Silent error swallowing in listHooksDetailed (HooksManager.ts)

try {
    configuration = await this.hookCache.getConfiguration(...);
    parsed = parseHookConfiguration(configuration);
} catch {
    parsed = { configured: false };   // error discarded, no log
}

A failed getHookConfiguration (e.g. an IAM permission error) is indistinguishable from a genuinely unconfigured hook — both surface as configured: false. That will make field debugging painful. Recommend logging the caught error at debug/warn before falling back.

Optional polish

  • describeHook has no in-flight dedup. Concurrent describeHook calls for the same uncached key will each hit cfnService.describeHook. listHooksDetailed already dedups configuration fetches via HookCache; the same in-flight-promise pattern could be applied here if the LSP can issue concurrent describes.
  • .refine((data) => data.typeName ?? data.arn, ...) (Describe/Deactivate schemas) works because NonEmptyZodString guarantees truthiness, but Boolean(data.typeName || data.arn) states the "at least one present" intent more clearly.
  • CreateGuardHookParamsSchema validates required keys are present (obj[key] === undefined) but not that they're non-empty/typed — fine for a params-layer check, just confirming deeper validation happens downstream.

Strengths

  • .strict() on every schema rejects unknown keys — good hardening for LSP-facing input.
  • S3 bucket validation is genuinely thorough (length + consecutive-dot/dot-hyphen + IP-address-format rejection).
  • describeHook caches results under both typeName and arn, so subsequent lookups by either identifier hit cache.
  • parseHookConfiguration / extractRuleUri are defensively written against malformed JSON and both string/object ruleLocation shapes.

The only blocking item is logging the swallowed error in listHooksDetailed; everything else is optional.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants