Skip to content

feat(@helm/adapters): scaffold IssueTrackerAdapter interface and MockAdapter#7

Merged
lhpaul merged 5 commits into
developfrom
feature/session-5-block-1-issue-tracker-adapter
May 15, 2026
Merged

feat(@helm/adapters): scaffold IssueTrackerAdapter interface and MockAdapter#7
lhpaul merged 5 commits into
developfrom
feature/session-5-block-1-issue-tracker-adapter

Conversation

@lhpaul
Copy link
Copy Markdown
Owner

@lhpaul lhpaul commented May 15, 2026

Summary

  • Adds @helm/adapters package with IssueTrackerAdapter TypeScript interface, shared types (NormalizedItem, NormalizedEvent, ItemFilter), and an in-memory MockAdapter for use in tests
  • MockAdapter provides seed() / getComments() helpers and implements all 8 interface methods; setSubStage, setStatus, and comment throw if the item doesn't exist
  • 19 Vitest contract tests cover all methods including filter combinations, edge cases, and parseWebhook pass-through / unknown fallback

Test plan

  • pnpm --filter @helm/adapters test — all 19 tests pass
  • pnpm --filter @helm/adapters buildtsc --noEmit clean

🤖 Generated with Claude Code

Summary by CodeRabbit

  • New Features

    • Adapter system for integrating external issue trackers with normalized item and event formats
    • Mock adapter for local/dev use
  • Tests

    • Test suite validating Mock adapter behavior: retrieval, filtering, comments, state updates, and webhook normalization
  • Chores

    • New adapters workspace package and TypeScript configuration added

Review Change Stack

lhpaul and others added 2 commits May 15, 2026 16:08
… interface + types

New pure-TypeScript package with zero runtime deps of its own.

IssueTrackerAdapter interface (interface.ts):
  ensureSubStages(config: IssueTracker)  — idempotent setup
  getItem / listItems                    — reads, returns null on missing
  setSubStage / setStatus / comment      — writes
  parseWebhook (sync)                    — normalises raw webhook payloads
  registerWebhook                        — registers Helm's callback URL

NormalizedItem: externalId, title, subStage (WorkflowStage|null), status,
url. No EXTERNAL_ID_REGEX here — FS validation stays at ItemStore boundary.

NormalizedEvent discriminated union: item_created | item_updated (new
values, not diff) | comment_added | unknown (safe catch-all).

ItemFilter: optional status and subStage predicates for listItems().

WorkflowStage imported directly from @helm/workflow (not via @helm/shared
re-export) per convention for new packages. IssueTracker imported from
@helm/shared. @types/node added as devDep (tsc traverses shared source).

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Implements all 8 IssueTrackerAdapter methods in memory (Map-backed),
adds seed/getComments test helpers, and covers 19 contract scenarios
with Vitest. Updates index.ts to export MockAdapter.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 15, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: d06141bc-ac44-4f19-9a6f-52a7b5051026

📥 Commits

Reviewing files that changed from the base of the PR and between d73bd39 and 35c225d.

⛔ Files ignored due to path filters (1)
  • pnpm-lock.yaml is excluded by !**/pnpm-lock.yaml
📒 Files selected for processing (1)
  • packages/adapters/package.json

📝 Walkthrough

Walkthrough

Adds the @helm/adapters workspace package: TypeScript types (NormalizedItem, ItemFilter, NormalizedEvent), IssueTrackerAdapter interface, public barrel export, in-memory MockAdapter implementation with tests, and package/tsconfig metadata.

Changes

Adapters Framework Package

Layer / File(s) Summary
Package setup and configuration
packages/adapters/package.json, packages/adapters/tsconfig.json
New @helm/adapters package declared private, exports ./src/index.ts, configures build/test/lint scripts, depends on @helm/shared and @helm/workflow. TypeScript compilation targets ./dist with ./src as root.
Shared type contracts
packages/adapters/src/types.ts
NormalizedItem defines tracker-agnostic item shape with externalId, title, nullable subStage, status, and url. ItemFilter offers optional status and subStage filters. NormalizedEvent union covers item_created, item_updated, comment_added, and fallback unknown with raw payload.
IssueTrackerAdapter interface
packages/adapters/src/interface.ts
Core contract specifying async methods for item retrieval (getItem, listItems), mutations (setSubStage, setStatus, comment), lifecycle (ensureSubStages, registerWebhook), and a synchronous parseWebhook that must not throw and returns an unknown fallback when unrecognized.
Public API exports
packages/adapters/src/index.ts
Barrel export re-exporting IssueTrackerAdapter type, NormalizedItem/NormalizedEvent/ItemFilter types, and MockAdapter implementation.
MockAdapter implementation
packages/adapters/src/mock-adapter.ts
In-memory implementation storing normalized items and comments in Maps. Supports seed/retrieval, list filtering by status/subStage, mutations with item-existence validation, comment storage with isolation guarantees, and webhook parsing with a minimal shape guard (pass-through or { type: 'unknown', raw }).
MockAdapter test suite
packages/adapters/src/mock-adapter.test.ts
Vitest suite covering getItem nullability and seeded retrieval, listItems with individual and combined filters, setSubStage/setStatus mutations and error handling, comment appending and copy semantics, parseWebhook shape detection and fallback, and ensureSubStages/registerWebhook success assertions.
sequenceDiagram
  participant Developer
  participant MockAdapter
  participant Internal as Internal_Maps
  participant TestSuite
  Developer->>MockAdapter: seed(item)
  MockAdapter->>Internal: store NormalizedItem
  TestSuite->>MockAdapter: getItem(externalId)
  MockAdapter->>Internal: lookup Map
  MockAdapter-->>TestSuite: NormalizedItem or null
  TestSuite->>MockAdapter: listItems(filter?)
  MockAdapter->>Internal: filter by status/subStage
  MockAdapter-->>TestSuite: NormalizedItem[]
  TestSuite->>MockAdapter: setSubStage(externalId, stage)
  MockAdapter->>Internal: validate & update
  MockAdapter-->>TestSuite: resolve or throw
  TestSuite->>MockAdapter: parseWebhook(rawEvent)
  MockAdapter->>MockAdapter: shape check
  MockAdapter-->>TestSuite: NormalizedEvent
Loading

🎯 2 (Simple) | ⏱️ ~12 minutes

Possibly related PRs:

  • lhpaul/helm#4: Introduced the canonical WorkflowStage types referenced by the adapter interface and types.
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and specifically summarizes the main changes: introducing the IssueTrackerAdapter interface and MockAdapter implementation for the new @helm/adapters package.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feature/session-5-block-1-issue-tracker-adapter

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@packages/adapters/src/mock-adapter.ts`:
- Around line 22-33: getItem and listItems return direct references from
this.items, allowing callers to mutate adapter state; change both to return
defensive copies instead. In getItem (function getItem) return a clone of the
found NormalizedItem (or null) instead of the stored object; in listItems
(function listItems) return an array of clones (apply the same filtering logic
but map to clones before returning). Use a safe cloning approach (e.g.,
structuredClone or an equivalent deep-clone utility) so external callers cannot
modify the internal this.items state and must use setSubStage/setStatus to
change items.
- Around line 60-69: The shape guard in parseWebhook is too permissive: ensure
the rawEvent.type is a string before casting to NormalizedEvent. Update
parseWebhook to check that rawEvent is an object, has a 'type' property, and
that (rawEvent as { type: unknown }).type is typeof 'string' and not equal to
'unknown' (e.g., add typeof (rawEvent as { type: unknown }).type === 'string' &&
(rawEvent as { type: string }).type !== 'unknown'); only then return rawEvent as
NormalizedEvent, otherwise return the fallback { type: 'unknown', raw: rawEvent
}.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 523ac6b4-6274-4b93-ad7f-63e5a9f3c2e8

📥 Commits

Reviewing files that changed from the base of the PR and between a003646 and c805a45.

⛔ Files ignored due to path filters (1)
  • pnpm-lock.yaml is excluded by !**/pnpm-lock.yaml
📒 Files selected for processing (7)
  • packages/adapters/package.json
  • packages/adapters/src/index.ts
  • packages/adapters/src/interface.ts
  • packages/adapters/src/mock-adapter.test.ts
  • packages/adapters/src/mock-adapter.ts
  • packages/adapters/src/types.ts
  • packages/adapters/tsconfig.json

Comment thread packages/adapters/src/mock-adapter.ts Outdated
…type guard

- getItem/listItems now return shallow copies to prevent external mutation
  of internal Map state
- parseWebhook now checks typeof type === 'string' before pass-through,
  rejecting numeric/non-string type values as unknown events
- Added test covering numeric type value edge case

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@packages/adapters/src/mock-adapter.ts`:
- Around line 63-70: The parseWebhook implementation can still throw when
accessing properties on hostile objects; update the parseWebhook function to
wrap the existing detection/destructuring logic in a try-catch so any thrown
error is swallowed and the function always returns a NormalizedEvent; on error
(or when checks fail) return { type: 'unknown', raw: rawEvent } as the fallback
per the adapter contract, keeping NormalizedEvent and parseWebhook identifiers
unchanged.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 1220b972-4458-4c4f-be87-738ca00f2c1c

📥 Commits

Reviewing files that changed from the base of the PR and between c805a45 and 8e7a4ee.

📒 Files selected for processing (2)
  • packages/adapters/src/mock-adapter.test.ts
  • packages/adapters/src/mock-adapter.ts

Comment thread packages/adapters/src/mock-adapter.ts
…throw contract

The interface requires parseWebhook to never throw. Property access on Proxy
objects (or objects with throwing getters) can still raise even after the
typeof check. Wrap in try-catch and fall back to {type:'unknown'} on any error.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@lhpaul
Copy link
Copy Markdown
Owner Author

lhpaul commented May 15, 2026

@CodeRabbit are we clean?

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 15, 2026

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

Let me take a thorough look at the code before giving you a verdict.


🏁 Script executed:

#!/bin/bash
echo "=== package.json ==="
cat packages/adapters/package.json

echo ""
echo "=== tsconfig.json ==="
cat packages/adapters/tsconfig.json

echo ""
echo "=== src/index.ts ==="
cat packages/adapters/src/index.ts

echo ""
echo "=== src/types.ts ==="
cat packages/adapters/src/types.ts

echo ""
echo "=== src/interface.ts ==="
cat packages/adapters/src/interface.ts

echo ""
echo "=== src/mock-adapter.ts ==="
cat packages/adapters/src/mock-adapter.ts

echo ""
echo "=== src/mock-adapter.test.ts ==="
cat packages/adapters/src/mock-adapter.test.ts

echo ""
echo "=== root package.json / workspace config ==="
cat package.json 2>/dev/null || echo "not found"
cat pnpm-workspace.yaml 2>/dev/null || echo "pnpm-workspace.yaml not found"

echo ""
echo "=== tsconfig.base.json ==="
cat tsconfig.base.json 2>/dev/null || cat tsconfig.json 2>/dev/null || echo "not found"

echo ""
echo "=== `@helm/shared` and `@helm/workflow` package locations ==="
fd "package.json" packages/ --max-depth 2 | xargs grep -l '"name"' | xargs grep -l '"@helm/'

Length of output: 14342


Let me quickly verify the WorkflowStage type to make sure the test fixtures are valid.


🏁 Script executed:

#!/bin/bash
echo "=== `@helm/workflow` types ==="
cat packages/workflow/src/index.ts 2>/dev/null || fd "index.ts" packages/workflow/src/ | xargs cat
echo ""
echo "=== WorkflowStage usages ==="
rg "WorkflowStage" packages/workflow/src/ -n
echo ""
echo "=== workflow types file ==="
fd "types.ts" packages/workflow/src/ | xargs cat 2>/dev/null
fd "stage" packages/workflow/src/ | xargs cat 2>/dev/null
echo ""
echo "=== pnpm shamefully-hoist config ==="
cat .npmrc 2>/dev/null || echo "no .npmrc"
cat packages/adapters/.npmrc 2>/dev/null || echo "no package-level .npmrc"

Length of output: 3220


@lhpaul Yes, the PR is clean — the interface contract is solid, the MockAdapter logic is correct, all 19 test cases look right, and the WorkflowStage values used in fixtures ('discovery', 'spec-ready', 'plan-ready') are valid members of the canonical WORKFLOW_STAGES tuple. A few minor nits worth being aware of, none blocking:


🟡 Minor observations

1. vitest not declared in package-level devDependencies
packages/adapters/package.json omits vitest, relying on pnpm's root-level hoisting. This works with pnpm@10's defaults (no .npmrc found), but if shamefully-hoist=false is ever added, pnpm --filter @helm/adapters test could break. Consider adding:

"devDependencies": {
  "@types/node": "^22.19.19",
  "vitest": "^2.1.9"
}

2. ensureSubStages() and registerWebhook() omit required interface params
The interface declares:

ensureSubStages(config: IssueTracker): Promise<void>;
registerWebhook(callbackUrl: string): Promise<void>;

…but MockAdapter implements both as zero-argument no-ops. TypeScript's structural subtyping permits this (fewer-param function is assignable to a more-param function type), so there's no compile error — but it means any config or callbackUrl passed via the interface type is silently discarded. This is acceptable for a test double, but worth a brief _config / _callbackUrl parameter (underscore-prefixed) to make the intentional drop explicit.

3. @types/node likely unnecessary
No Node.js built-in APIs are used in src/. The dep is harmless but can be dropped to keep the manifest lean.


Everything else — the type design, the defensive parseWebhook try-catch, copy-on-read semantics in getItem/listItems/getComments, and the error messages — looks well thought out. Good to merge as-is if you're fine with the above.

vitest was relying on pnpm root-level hoisting; declare it explicitly so
the package is self-contained if shamefully-hoist is ever disabled.
@types/node has no callers in src/ and can be removed.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@lhpaul lhpaul merged commit cb38d1f into develop May 15, 2026
1 check passed
@lhpaul lhpaul deleted the feature/session-5-block-1-issue-tracker-adapter branch May 15, 2026 20:37
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.

1 participant