Update workflows to create Repository/User attribution nodes via StorageAdapter #1452
Conversation
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
…Adapter and update webhook types\n\n- Expand GitHub Issues webhook payload schema to include full repository and sender metadata\n- Replace initializeWorkflowRun with StorageAdapter.workflow.run.create in resolveIssue workflow\n- Add actor attribution (webhook/user) and pass full repository metadata to storage\n- Pass webhook context from issue label handler (sender id/login, installation id)\n- Use neo4jDs singleton in worker orchestrator and explicit READ sessions for settings\n- Update action files to use neo4jDs.getSession('READ') consistently\n- Remove deprecated linkToIssue from Neo4j repositories; mark listWorkflowRuns as @deprecated\n\nNotes:\n- Visibility is normalized to PUBLIC/PRIVATE/INTERNAL\n- Prepared for optional commit attribution support via StorageAdapter (not yet provided here)
Contributor
WalkthroughThis PR refactors workflow run persistence by introducing a modular StorageAdapter with separate attachment operations, removes legacy Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Caller
participant SA as StorageAdapter
participant Query as Neo4j Queries
participant DB as Neo4j Database
Caller->>SA: workflow.run.create(input)
activate SA
SA->>Query: createWorkflowRun(tx, core params)
activate Query
Query->>DB: CREATE WorkflowRun node
DB-->>Query: new WorkflowRun
Query-->>SA: CreateWorkflowRunResult
deactivate Query
alt Has Attachments
SA->>Query: attachRepository(tx, repo)
activate Query
Query->>DB: MERGE Repository, link BASED_ON_REPO
DB-->>Query: success
Query-->>SA: AttachRepositoryResult
deactivate Query
SA->>Query: attachIssue(tx, issue)
activate Query
Query->>DB: MERGE Issue, link BASED_ON_ISSUE
DB-->>Query: success
Query-->>SA: AttachIssueResult
deactivate Query
SA->>Query: attachActor(tx, actor)
activate Query
Query->>DB: MERGE Actor, link INITIATED_BY
DB-->>Query: success
Query-->>SA: AttachActorResult
deactivate Query
SA->>Query: attachCommit(tx, commit)
activate Query
Query->>DB: MERGE Commit, link BASED_ON_COMMIT
DB-->>Query: success
Query-->>SA: AttachCommitResult
deactivate Query
end
SA->>Query: getWorkflowRunById(tx, runId)
activate Query
Query->>DB: MATCH complete workflow graph
DB-->>Query: GetWorkflowRunByIdResult
Query-->>SA: mapped WorkflowRun
deactivate Query
SA-->>Caller: WorkflowRunHandle with run + add/attach APIs
deactivate SA
Possibly related issues
Possibly related PRs
✨ Finishing touches
Comment |
Owner
|
Let's make sure we also have test coverage of these code changes using jest --coverage. |
Owner
|
As we're migrating away from some functions, such as initializeWorkflowRun, let's mark them as @deprecated with directions on which new functions to use using jsdoc. |
…nder schema; deprecate initializeWorkflowRun\n\n- Update webhook route tests to include required repository.id/node_id/name/owner.login and sender.id fields per new IssuesPayloadSchema\n- Add JSDoc @deprecated to initializeWorkflowRun with guidance to use StorageAdapter.workflow.run.create\n- Mark listWorkflowRuns as @deprecated in shared services for consistency\n\nThis addresses review feedback to ensure coverage reflects the schema change and to annotate deprecated functions with migration paths.
Owner
|
there's still quite a few workflows that are using the deprecated |
Owner
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
youngchingjui
deleted the
feature/workflows-repo-user-attribution-nodes
branch
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 16
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
shared/src/adapters/neo4j/types.ts (1)
113-129: Risk: Making immutable identifiers optional undermines data integrity.Lines 118-119 make
idandnodeIdoptional, but the comment on line 115 explicitly states these are "Immutable identifiers." Making unique identifiers optional could lead to:
- Repository nodes without stable identifiers, making future matching/merging unreliable
- Potential duplicate nodes for the same repository
- Difficulty in enforcing uniqueness constraints
If repositories are created without these IDs initially, how will they be matched and merged when the IDs become available later? Consider requiring at least one immutable identifier at creation time.
💡 Suggested approach
If progressive attachment is needed, consider requiring
fullNameas the minimum identifier (since it's marked required) and making onlynodeIdoptional. The numericidshould be required when available from GitHub API responses.Alternatively, document the matching/merging strategy for repositories created without immutable identifiers.
shared/src/adapters/neo4j/queries/workflowRuns/listForRepo.ts (1)
21-47:statecan be null but TS type says it can’t.Because the workflowState event match is optional,
latestWorkflowState(and thuslatestWorkflowState.state) can be null. ButListForRepoResult.stateis typed asWorkflowRunState(non-null).Proposed fix (default at query level)
- RETURN w, userActor, webhookEvent, webhookSender, latestWorkflowState.state AS state, i, r, c + RETURN w, userActor, webhookEvent, webhookSender, + coalesce(latestWorkflowState.state, 'pending') AS state, + i, r, c
🤖 Fix all issues with AI agents
In @__tests__/shared/adapters/neo4j/StorageAdapter.neo4j.test.ts:
- Around line 132-145: The describe block name is outdated
("workflow.run.listEvents") and should match the API under test; update the
describe string to "workflow.events.list" so it reflects the calls to
adapter.workflow.events.list in the tests (locate the describe that wraps the
two it(...) cases and change its title to "workflow.events.list").
In @__tests__/shared/adapters/neo4j/workflowRuns.neo4j.test.ts:
- Around line 29-46: The cleanup code is deleting Repository nodes by repo.id
but the tests create repos with fullName like "test/prog-repo-1", so change the
cleanup to delete by repo.fullName (e.g., MATCH (repo:Repository) WHERE
repo.fullName IN $repoFullNames DETACH DELETE repo) and pass
TEST_DATA.repositories as the $repoFullNames parameter, or alternatively update
TEST_DATA.repositories to contain the actual repo id values created by the
tests; apply the same fix to the other cleanup locations referenced (the other
uses of repo.id IN $repoIds).
- Around line 148-191: The test asserts handle.run.state === "pending" even
though WorkflowRun.state is event-sourced and not stored on the node; remove or
replace that brittle assertion: either drop the state check entirely in the
"Minimal workflow run creation" test (the remaining id/type checks and
relationship counts suffice) or fetch the canonical computed view (e.g., via
adapter.workflow.run.getById or the event-sourced projection) and assert its
computed state there; update references to handle.run.state in this test
accordingly so you don't rely on a non-persisted field.
- Around line 338-379: The test currently only checks events.length but not
order; update the assertion after calling
adapter.workflow.events.list(handle.run.id) to verify sequence by inspecting the
returned events array (from adapter.workflow.events.list) for ordering—e.g., map
to event payload content or order/timestamp fields and assert that the sequence
equals ["Event 1","Event 2","Event 3"] (or assert events[0].payload.content ===
"Event 1", events[1].payload.content === "Event 2", events[2].payload.content
=== "Event 3"). Ensure you reference the returned events from
adapter.workflow.events.list(handle.run.id) and keep the cleanup block
unchanged.
In @lib/webhook/github/handlers/issue/label.resolve.handler.ts:
- Line 32: labelerId is being extracted from payload but never used; decide
whether it should be passed into resolveIssue or added to the webhook actor
context for StorageAdapter workflow runs and implement accordingly. If labelerId
should be actor attribution, include it in the call that starts the workflow
(e.g., pass an actor or metadata object containing labelerId into resolveIssue
invocation or into the context passed to StorageAdapter.workflow.run.create) so
downstream code can attribute actions; if it is not needed yet, remove the const
labelerId = payload.sender?.id extraction to avoid dead code. Ensure you update
any call sites that construct the webhook context or resolveIssue parameters to
include labelerId under a clear key like actorId/labelerId.
In @shared/src/adapters/neo4j/queries/workflowRuns/addEvent.mapper.ts:
- Around line 148-152: Wrap the `default` switch clause in a block to satisfy
the Biome `noSwitchDeclarations` rule: replace the current `default:` section
with `default: { ... }` and move the `_exhaustive` declaration and the `throw
new Error(\`Unknown event type: ${(_exhaustive as AnyEvent).type}\`)` inside
that block so they are not declared directly in the switch scope; ensure you
keep the same `_exhaustive` symbol and `AnyEvent` cast and close the block.
In @shared/src/adapters/neo4j/queries/workflowRuns/addEvent.ts:
- Around line 8-33: Change the CREATE of the event node to a MERGE keyed by id
to make creation idempotent: replace the CREATE (newEvent:Event { ... }) with
MERGE (newEvent:Event { id: $eventId }) and move property assignments into an ON
CREATE SET block (set newEvent.type, newEvent.createdAt = datetime($createdAt),
and newEvent.content) so retries don't create duplicate Event nodes or reassign
timestamps; ensure all references (newEvent, lastEvent, wr) and the RETURN still
use the merged newEvent. Also apply the same MERGE + ON CREATE SET pattern to
the other identical event-creation block mentioned in the diff.
In @shared/src/adapters/neo4j/queries/workflowRuns/attachActor.ts:
- Around line 12-14: The queries USER_ACTOR_QUERY, GITHUB_USER_ACTOR_QUERY and
WEBHOOK_ACTOR_QUERY in attachActor only delete their own relationship type,
leaving stale actor relations; update each query so the pre-creation cleanup
OPTIONAL MATCH captures both INITIATED_BY and TRIGGERED_BY (e.g., OPTIONAL MATCH
(wr)-[oldRel:INITIATED_BY|TRIGGERED_BY]->() ) and DELETE oldRel so any existing
actor relationship is removed before creating the new one.
In @shared/src/adapters/neo4j/queries/workflowRuns/attachCommit.ts:
- Around line 8-21: The MERGE for Commit only sets commit.nodeId and
commit.message under ON CREATE so later attachCommit calls can't backfill
missing metadata; modify the query in attachCommit to always set or coalesce
these properties after the MERGE (e.g., add a SET clause that assigns
commit.nodeId = coalesce(commit.nodeId, $commitNodeId) and commit.message =
coalesce(commit.message, $commitMessage) or conditionally set when params are
not null) so subsequent calls update missing fields; apply the same change to
the other similar block referenced (lines 36-45).
In @shared/src/adapters/neo4j/queries/workflowRuns/createWorkflowRun.mapper.ts:
- Around line 15-21: The mapCreateWorkflowRunResult function currently declares
params but never uses it and accesses result.records[0] without checking for an
empty result; remove the unused params parameter (or document its intended
future use) and add a defensive null/length check on result.records before
accessing index 0 (e.g., ensure a record exists and throw or return a clear
error if not), then safely obtain wrNode from the validated record and call
workflowRunSchema.parse on its properties; ensure any thrown error includes
context about the createWorkflowRun query for easier debugging.
In @shared/src/adapters/neo4j/queries/workflowRuns/createWorkflowRun.ts:
- Around line 15-23: The CREATE of WorkflowRun is not idempotent; change the
Cypher in QUERY (and the other occurrence around the second block) to MERGE on
wr by id ($runId) and use ON CREATE SET to assign properties like type,
postToGithub, and createdAt so they are set only when a new node is created, and
use ON MATCH SET if you want to update mutable fields on duplicate deliveries;
update the query constants referenced (e.g., QUERY and the similar query at
lines 46-55) accordingly.
In @shared/src/adapters/neo4j/queries/workflowRuns/getWorkflowRunById.mapper.ts:
- Around line 26-76: The mapActor function should not coerce missing
webhookEvent.action to an empty string; update the webhook branch in mapActor to
set action to webhookEvent.action (which will be undefined when absent) instead
of using `?? ""`, and ensure the WorkflowRunActor type/interface accepts an
optional action so no runtime or type errors occur.
- Around line 98-131: The commit mapping uses a non-null assertion repository!
which will throw if a commitNode exists but repoNode is missing; update the
commit mapping in getWorkflowRunById mapper to avoid the non-null assertion by
making commit.repository optional or deriving a repository value from the commit
properties (e.g., use parsed.repository/repoFullName from commitSchema if
present) and only set repository when you have valid data, or alternatively
ensure upstream query always returns the repo when a commit exists (fetch
(commit)-[:IN_REPOSITORY]->(repo)) if domain requires a repository.
- Around line 95-143: The mapper is incorrectly deriving state from wr.state
(workflowRun node) which is not stored there; update the getWorkflowRunById
Cypher to also MATCH the latest workflowState Event (same pattern used in
listForRepo/listForIssue/listByUser: `(w)-[:STARTS_WITH|NEXT*]->(e:Event {type:
'workflowState'})` and return it as latestWorkflowState) and pass that node into
this mapper; then change the mapping logic to use
latestWorkflowState.properties.state (or its parsed event via the event schema)
instead of wr.state (so replace `state: wr.state ?? "pending"` with the derived
`state` from latestWorkflowState, falling back to "pending" if missing). Ensure
the mapper references the returned latestWorkflowState symbol and parses it
consistently with other mappers.
🧹 Nitpick comments (10)
shared/src/entities/Queue.ts (1)
3-4: Consider resolving the architectural placement questions.The TODOs raise valid concerns about whether queue definitions belong closer to their consumers (neo4j adapters) or with related channel definitions. Since this file is now part of the public entity surface, clarifying its placement would improve maintainability.
Consider:
- If queues are tightly coupled to Neo4j operations, moving to
shared/src/adapters/neo4j/might improve cohesion- If queues are a general messaging concept, keeping them separate or in a
shared/src/entities/messaging/namespace might be clearer- Document the decision to help future contributors understand the rationale
__tests__/api/webhook/github/route.test.ts (1)
245-276: Consider adding test coverage for optional repository fields.The test payloads currently omit the optional repository fields (
default_branch,visibility,has_issues). While the schema correctly handles this, adding test cases that include these fields would improve coverage and document their expected behavior in the webhook flow.📝 Example test case for optional fields
it("routes i2pr resolve issue label with full repository metadata", async () => { const payload = { action: "labeled", label: { name: "i2pr: resolve issue" }, repository: { id: 123, node_id: "R_kgDOTest", full_name: "octo-org/octo-repo", name: "octo-repo", owner: { login: "octo-org" }, default_branch: "main", visibility: "public" as const, has_issues: true, }, issue: { number: 42 }, sender: { id: 1001, login: "octocat" }, installation: { id: 9876 }, } const mockRequest = createSignedRequest(payload, "issues") const response = await POST(mockRequest) expect(response.status).toBe(200) expect(handleIssueLabelAutoResolve).toHaveBeenCalledWith( expect.objectContaining({ payload: expect.objectContaining({ repository: expect.objectContaining({ default_branch: "main", visibility: "public", has_issues: true, }), }), }) ) })shared/src/adapters/neo4j/queries/workflowRuns/listByUser.ts (2)
1-14: Consider completing the type-only import migration for consistency.While
QueryResultand the domain types (Commit,Issue,User, etc.) were correctly converted to type-only imports,Integer,ManagedTransaction, andNodeare also only used in type positions and could be converted for consistency.♻️ Optional: Complete type-only import migration
import { - Integer, - ManagedTransaction, - Node, + type Integer, + type ManagedTransaction, + type Node, type QueryResult, } from "neo4j-driver"
22-25: Verification complete: all downstream consumers properly updated.The removal of the
LINKED_GITHUB_USERrelationship andghfield is safe:
- No references to the removed
ghfield exist in the codebase- The
ListByUserResultinterface (lines 32-39) and query correctly omitgh- The mapper and tests use only the new result shape without issues
- Existing Neo4j data is unaffected (removal is query-only)
Consider refactoring:
shared/src/adapters/neo4j/queries/workflowRuns/index.tsis a barrel file re-exporting symbols fromlistByUserandlistByUser.mapper. Import from original module paths directly to maintain clearer dependency graphs and better tree-shaking.shared/src/adapters/neo4j/queries/workflowRuns/listForIssue.ts (1)
1-13: Consider completing the type-only import migration for consistency.Similar to
listByUser.ts,Integer,ManagedTransaction, andNodeare only used in type positions and could be converted to type-only imports for consistency with the other type-only imports in this file.♻️ Optional: Complete type-only import migration
import { - Integer, - ManagedTransaction, - Node, + type Integer, + type ManagedTransaction, + type Node, type QueryResult, } from "neo4j-driver"__tests__/shared/adapters/neo4j/testUtils.ts (1)
138-142: Minor: Label parameter interpolation in Cypher.While this is a test utility and
labelis controlled internally, directly interpolating into Cypher queries is a pattern to avoid. Consider using a parameterized approach or validating the label against an allowlist.Suggested improvement
+const VALID_LABELS = ["WorkflowRun", "Repository", "Issue", "Commit", "Actor"] as const + export async function getNodeCount( ds: Neo4jDataSource, label: string ): Promise<number> { + if (!VALID_LABELS.includes(label as typeof VALID_LABELS[number])) { + throw new Error(`Invalid label: ${label}`) + } const session = ds.getSession("READ")shared/src/adapters/neo4j/queries/workflowRuns/attachRepository.ts (1)
15-18: Inconsistent update strategy for derived vs source fields.The
ON MATCHblock updatesfullNameunconditionally (line 15) but usescoalesceforownerandname(lines 17-18). SincefullNameis derived fromownerandname(line 55), this creates a potential inconsistency: if the incomingrepoOwnerorrepoNameare empty/null but protected bycoalesce, the repository will retain its oldowner/namebut get a freshly computedfullNamethat doesn't match them.Consider one of these approaches:
- Option 1 (recommended): Also apply
coalescetofullNamefor consistency:repo.fullName = coalesce($repoFullName, repo.fullName)- Option 2: Remove
coalescefromownerandnameif the intent is to always overwrite with fresh webhook data.Proposed fix: Apply coalesce to fullName
ON MATCH SET repo.fullName = $repoFullName, + repo.fullName = coalesce($repoFullName, repo.fullName), repo.githubInstallationId = coalesce($repoGithubInstallationId, repo.githubInstallationId), - repo.owner = coalesce($repoOwner, repo.owner), - repo.name = coalesce($repoName, repo.name) + repo.owner = $repoOwner, + repo.name = $repoName(Choose either approach depending on your update semantics)
shared/src/adapters/neo4j/StorageAdapter.ts (1)
167-176: Consider clarifying the error message for partial repository data.The error message on line 173-175 suggests using
handle.attach.repository()instead, but this function is called by bothhandle.attach.target()and internally byrunCreate. The message could be more helpful by explaining that all required fields (id, owner, name) must be provided.Suggested improvement
throw new Error( - "Repository attachment via target requires repository.id + owner + name - use handle.attach.repository() instead" + "Repository attachment requires repository.id, owner, and name to be provided. " + + "Either supply all fields in target.repository or use handle.attach.repository() with complete data." )shared/src/adapters/neo4j/queries/workflowRuns/index.ts (1)
1-18: Avoid barrel file pattern per coding guidelines.This file acts as a barrel file whose sole purpose is to re-export symbols from multiple modules. As per the coding guidelines, prefer importing from original module paths to maintain clear dependency graphs and enable better tree-shaking.
Consider removing this index file and updating imports throughout the codebase to reference the original module paths directly (e.g.,
import { createWorkflowRun } from "@/shared/adapters/neo4j/queries/workflowRuns/createWorkflowRun"instead ofimport { createWorkflowRun } from "@/shared/adapters/neo4j/queries/workflowRuns").As per coding guidelines.
shared/src/ports/db/index.ts (1)
36-40: Consider stronger typing for payload field.The
payloadfield is typed asunknown, which bypasses TypeScript's type checking and could allow invalid event payloads to pass through. SinceAllEventsis a discriminated union with specific payload structures for each event type, consider using a more specific type or implementing runtime validation.Given that the PR already uses Zod schemas for webhook payload validation (per PR objectives), consider creating Zod schemas for these input types as well to ensure type safety at both compile-time and runtime.
Possible approaches
Option 1: Use a discriminated union approach:
export type WorkflowEventInput = { [K in AllEvents["type"]]: { type: K payload: Extract<AllEvents, { type: K }>["payload"] createdAt?: string } }[AllEvents["type"]]Option 2: Create Zod schemas for validation (aligning with webhook payload patterns in the PR):
export const WorkflowEventInputSchema = z.object({ type: z.enum([/* event types */]), payload: z.unknown(), // then validate based on type createdAt: z.string().optional(), })
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (42)
.gitignore__tests__/.env.example__tests__/README.md__tests__/api/webhook/github.route.test.ts__tests__/api/webhook/github/route.test.ts__tests__/shared/adapters/neo4j/StorageAdapter.neo4j.test.ts__tests__/shared/adapters/neo4j/queries.neo4j.test.ts__tests__/shared/adapters/neo4j/testUtils.ts__tests__/shared/adapters/neo4j/workflowRuns.neo4j.test.tsapps/workers/workflow-workers/src/orchestrators/autoResolveIssue.tslib/neo4j/repositories/workflowRun.tslib/neo4j/services/workflow.tslib/webhook/github/handlers/issue/label.resolve.handler.tslib/webhook/github/types.tslib/workflows/resolveIssue.tsshared/src/adapters/neo4j/StorageAdapter.tsshared/src/adapters/neo4j/queries/workflowRuns/addEvent.mapper.tsshared/src/adapters/neo4j/queries/workflowRuns/addEvent.tsshared/src/adapters/neo4j/queries/workflowRuns/attachActor.tsshared/src/adapters/neo4j/queries/workflowRuns/attachCommit.tsshared/src/adapters/neo4j/queries/workflowRuns/attachIssue.tsshared/src/adapters/neo4j/queries/workflowRuns/attachRepository.tsshared/src/adapters/neo4j/queries/workflowRuns/createWorkflowRun.mapper.tsshared/src/adapters/neo4j/queries/workflowRuns/createWorkflowRun.tsshared/src/adapters/neo4j/queries/workflowRuns/getWorkflowRunById.mapper.tsshared/src/adapters/neo4j/queries/workflowRuns/getWorkflowRunById.tsshared/src/adapters/neo4j/queries/workflowRuns/index.tsshared/src/adapters/neo4j/queries/workflowRuns/listByUser.mapper.tsshared/src/adapters/neo4j/queries/workflowRuns/listByUser.tsshared/src/adapters/neo4j/queries/workflowRuns/listEvents.mapper.tsshared/src/adapters/neo4j/queries/workflowRuns/listEvents.tsshared/src/adapters/neo4j/queries/workflowRuns/listForIssue.mapper.tsshared/src/adapters/neo4j/queries/workflowRuns/listForIssue.tsshared/src/adapters/neo4j/queries/workflowRuns/listForRepo.mapper.tsshared/src/adapters/neo4j/queries/workflowRuns/listForRepo.tsshared/src/adapters/neo4j/types.tsshared/src/entities/Commit.tsshared/src/entities/Queue.tsshared/src/entities/WorkflowRun.tsshared/src/lib/neo4j/services/workflow.tsshared/src/ports/db/index.tsshared/src/usecases/workflows/autoResolveIssue.ts
💤 Files with no reviewable changes (1)
- tests/api/webhook/github.route.test.ts
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js,jsx,mts,mjs}
📄 CodeRabbit inference engine (.cursor/rules/code-structure.mdc)
Avoid barrel files: do not create files whose sole purpose is to re-export symbols from multiple modules; prefer importing from original module paths to maintain clear dependency graphs and better tree-shaking
Files:
shared/src/entities/Commit.ts__tests__/shared/adapters/neo4j/workflowRuns.neo4j.test.tsshared/src/entities/WorkflowRun.tslib/neo4j/repositories/workflowRun.tsshared/src/adapters/neo4j/queries/workflowRuns/createWorkflowRun.tsshared/src/adapters/neo4j/queries/workflowRuns/getWorkflowRunById.ts__tests__/api/webhook/github/route.test.ts__tests__/shared/adapters/neo4j/StorageAdapter.neo4j.test.tsshared/src/adapters/neo4j/queries/workflowRuns/addEvent.mapper.tsshared/src/adapters/neo4j/queries/workflowRuns/getWorkflowRunById.mapper.tsshared/src/adapters/neo4j/queries/workflowRuns/createWorkflowRun.mapper.tsshared/src/adapters/neo4j/queries/workflowRuns/attachCommit.ts__tests__/shared/adapters/neo4j/testUtils.tslib/neo4j/services/workflow.tsshared/src/adapters/neo4j/queries/workflowRuns/addEvent.tsshared/src/adapters/neo4j/queries/workflowRuns/listEvents.mapper.tslib/webhook/github/handlers/issue/label.resolve.handler.tsshared/src/adapters/neo4j/queries/workflowRuns/attachRepository.tsshared/src/adapters/neo4j/queries/workflowRuns/listByUser.tsshared/src/adapters/neo4j/queries/workflowRuns/listByUser.mapper.tsshared/src/ports/db/index.tsshared/src/adapters/neo4j/queries/workflowRuns/listForIssue.mapper.tsshared/src/entities/Queue.tsshared/src/adapters/neo4j/queries/workflowRuns/listForIssue.tsshared/src/adapters/neo4j/queries/workflowRuns/listEvents.tslib/webhook/github/types.tsshared/src/adapters/neo4j/StorageAdapter.tsapps/workers/workflow-workers/src/orchestrators/autoResolveIssue.ts__tests__/shared/adapters/neo4j/queries.neo4j.test.tsshared/src/adapters/neo4j/queries/workflowRuns/index.tsshared/src/lib/neo4j/services/workflow.tslib/workflows/resolveIssue.tsshared/src/adapters/neo4j/queries/workflowRuns/attachIssue.tsshared/src/adapters/neo4j/types.tsshared/src/adapters/neo4j/queries/workflowRuns/listForRepo.tsshared/src/usecases/workflows/autoResolveIssue.tsshared/src/adapters/neo4j/queries/workflowRuns/listForRepo.mapper.tsshared/src/adapters/neo4j/queries/workflowRuns/attachActor.ts
🧠 Learnings (14)
📓 Common learnings
Learnt from: youngchingjui
Repo: youngchingjui/issue-to-pr PR: 1241
File: app/api/webhook/github/route.ts:85-90
Timestamp: 2025-09-12T01:44:51.070Z
Learning: In the webhook handlers at app/api/webhook/github/route.ts, runWithInstallationId is intentionally not awaited to allow fire-and-forget background processing of long-running workflows, avoiding webhook timeouts. The team plans to migrate these to proper queues/workers in a separate issue later.
Learnt from: youngchingjui
Repo: youngchingjui/issue-to-pr PR: 1282
File: lib/adapters/PersistingEventBusAdapter.ts:55-58
Timestamp: 2025-09-22T09:28:41.886Z
Learning: The project is migrating away from neo4j/services/event adapters, so issues in files like lib/adapters/PersistingEventBusAdapter.ts that use these adapters may be temporary and lower priority.
📚 Learning: 2026-01-02T09:21:42.011Z
Learnt from: youngchingjui
Repo: youngchingjui/issue-to-pr PR: 1450
File: shared/src/adapters/neo4j/types.ts:12-17
Timestamp: 2026-01-02T09:21:42.011Z
Learning: In the Neo4j data model at shared/src/adapters/neo4j/types.ts, the WorkflowRun node intentionally does not include a `state` field. The workflow run state is derived from the chain of events rather than being stored directly on the WorkflowRun node, following an event sourcing pattern where state is computed by replaying events.
Applied to files:
__tests__/shared/adapters/neo4j/workflowRuns.neo4j.test.tsshared/src/entities/WorkflowRun.tslib/neo4j/repositories/workflowRun.tsshared/src/adapters/neo4j/queries/workflowRuns/createWorkflowRun.tsshared/src/adapters/neo4j/queries/workflowRuns/getWorkflowRunById.ts__tests__/shared/adapters/neo4j/StorageAdapter.neo4j.test.tsshared/src/adapters/neo4j/queries/workflowRuns/addEvent.mapper.tsshared/src/adapters/neo4j/queries/workflowRuns/getWorkflowRunById.mapper.tsshared/src/adapters/neo4j/queries/workflowRuns/createWorkflowRun.mapper.tsshared/src/adapters/neo4j/queries/workflowRuns/attachCommit.tslib/neo4j/services/workflow.tsshared/src/adapters/neo4j/queries/workflowRuns/addEvent.tsshared/src/adapters/neo4j/queries/workflowRuns/listEvents.mapper.tsshared/src/adapters/neo4j/queries/workflowRuns/attachRepository.tsshared/src/adapters/neo4j/queries/workflowRuns/listByUser.tsshared/src/adapters/neo4j/queries/workflowRuns/listByUser.mapper.tsshared/src/ports/db/index.tsshared/src/adapters/neo4j/queries/workflowRuns/listForIssue.mapper.tsshared/src/entities/Queue.tsshared/src/adapters/neo4j/queries/workflowRuns/listForIssue.tsshared/src/adapters/neo4j/queries/workflowRuns/listEvents.tsshared/src/adapters/neo4j/StorageAdapter.tsapps/workers/workflow-workers/src/orchestrators/autoResolveIssue.tsshared/src/adapters/neo4j/queries/workflowRuns/index.tsshared/src/lib/neo4j/services/workflow.tsshared/src/adapters/neo4j/queries/workflowRuns/attachIssue.tsshared/src/adapters/neo4j/types.tsshared/src/adapters/neo4j/queries/workflowRuns/listForRepo.tsshared/src/usecases/workflows/autoResolveIssue.tsshared/src/adapters/neo4j/queries/workflowRuns/listForRepo.mapper.tsshared/src/adapters/neo4j/queries/workflowRuns/attachActor.ts
📚 Learning: 2025-09-22T09:28:41.886Z
Learnt from: youngchingjui
Repo: youngchingjui/issue-to-pr PR: 1282
File: lib/adapters/PersistingEventBusAdapter.ts:55-58
Timestamp: 2025-09-22T09:28:41.886Z
Learning: The project is migrating away from neo4j/services/event adapters, so issues in files like lib/adapters/PersistingEventBusAdapter.ts that use these adapters may be temporary and lower priority.
Applied to files:
__tests__/shared/adapters/neo4j/workflowRuns.neo4j.test.tslib/neo4j/repositories/workflowRun.ts__tests__/shared/adapters/neo4j/StorageAdapter.neo4j.test.tsshared/src/adapters/neo4j/queries/workflowRuns/addEvent.mapper.tslib/neo4j/services/workflow.tsshared/src/adapters/neo4j/queries/workflowRuns/addEvent.tsshared/src/adapters/neo4j/queries/workflowRuns/listEvents.mapper.tsshared/src/adapters/neo4j/queries/workflowRuns/listForIssue.mapper.tsshared/src/adapters/neo4j/queries/workflowRuns/listForIssue.tsshared/src/adapters/neo4j/queries/workflowRuns/listEvents.tsshared/src/adapters/neo4j/StorageAdapter.tsapps/workers/workflow-workers/src/orchestrators/autoResolveIssue.ts__tests__/shared/adapters/neo4j/queries.neo4j.test.tsshared/src/adapters/neo4j/queries/workflowRuns/index.tsshared/src/lib/neo4j/services/workflow.tsshared/src/adapters/neo4j/types.ts
📚 Learning: 2025-09-22T09:32:04.764Z
Learnt from: youngchingjui
Repo: youngchingjui/issue-to-pr PR: 1282
File: lib/types/db/neo4j.ts:89-91
Timestamp: 2025-09-22T09:32:04.764Z
Learning: The new WorkflowStateEventSchema in shared/src/entities/events/WorkflowEvent.ts is built from BaseFields (id, timestamp) and does not contain a workflowId field, so DB-level schemas should not attempt to omit workflowId when using this schema.
Applied to files:
shared/src/entities/WorkflowRun.tslib/neo4j/repositories/workflowRun.tsshared/src/adapters/neo4j/queries/workflowRuns/createWorkflowRun.tsshared/src/adapters/neo4j/queries/workflowRuns/addEvent.mapper.tsshared/src/adapters/neo4j/queries/workflowRuns/getWorkflowRunById.mapper.tsshared/src/adapters/neo4j/queries/workflowRuns/createWorkflowRun.mapper.tsshared/src/adapters/neo4j/queries/workflowRuns/addEvent.tsshared/src/adapters/neo4j/queries/workflowRuns/listEvents.mapper.tsshared/src/adapters/neo4j/queries/workflowRuns/listByUser.tsshared/src/adapters/neo4j/queries/workflowRuns/listByUser.mapper.tsshared/src/ports/db/index.tsshared/src/adapters/neo4j/queries/workflowRuns/listEvents.tsshared/src/adapters/neo4j/queries/workflowRuns/index.tsshared/src/lib/neo4j/services/workflow.tsshared/src/adapters/neo4j/types.tsshared/src/usecases/workflows/autoResolveIssue.ts
📚 Learning: 2025-09-22T09:32:04.764Z
Learnt from: youngchingjui
Repo: youngchingjui/issue-to-pr PR: 1282
File: lib/types/db/neo4j.ts:89-91
Timestamp: 2025-09-22T09:32:04.764Z
Learning: The new WorkflowStateEventSchema in shared/src/entities/events/WorkflowEvent.ts does not contain a workflowId field, so DB-level schemas should not attempt to omit it when using this schema.
Applied to files:
shared/src/entities/WorkflowRun.tsshared/src/adapters/neo4j/queries/workflowRuns/createWorkflowRun.tsshared/src/adapters/neo4j/queries/workflowRuns/addEvent.mapper.tsshared/src/adapters/neo4j/queries/workflowRuns/getWorkflowRunById.mapper.tsshared/src/adapters/neo4j/queries/workflowRuns/addEvent.tsshared/src/adapters/neo4j/queries/workflowRuns/listEvents.mapper.tsshared/src/adapters/neo4j/queries/workflowRuns/listByUser.tsshared/src/adapters/neo4j/queries/workflowRuns/listByUser.mapper.tsshared/src/ports/db/index.tsshared/src/adapters/neo4j/queries/workflowRuns/listEvents.tsshared/src/adapters/neo4j/queries/workflowRuns/index.tsshared/src/lib/neo4j/services/workflow.tsshared/src/adapters/neo4j/types.ts
📚 Learning: 2025-09-22T09:34:43.580Z
Learnt from: youngchingjui
Repo: youngchingjui/issue-to-pr PR: 1282
File: lib/neo4j/convert.ts:69-74
Timestamp: 2025-09-22T09:34:43.580Z
Learning: The file lib/neo4j/convert.ts is being moved away from/deprecated, so fixes in this file may not be worth implementing since it will be replaced.
Applied to files:
lib/neo4j/repositories/workflowRun.tslib/neo4j/services/workflow.tsshared/src/adapters/neo4j/queries/workflowRuns/listEvents.mapper.tsshared/src/adapters/neo4j/queries/workflowRuns/listForIssue.mapper.tsshared/src/adapters/neo4j/queries/workflowRuns/listForIssue.tsshared/src/adapters/neo4j/queries/workflowRuns/listEvents.tsshared/src/adapters/neo4j/StorageAdapter.ts__tests__/shared/adapters/neo4j/queries.neo4j.test.tsshared/src/lib/neo4j/services/workflow.tsshared/src/adapters/neo4j/types.ts
📚 Learning: 2025-09-22T09:33:51.194Z
Learnt from: youngchingjui
Repo: youngchingjui/issue-to-pr PR: 1282
File: lib/neo4j/services/event.ts:365-370
Timestamp: 2025-09-22T09:33:51.194Z
Learning: The file lib/neo4j/services/event.ts is planned for migration/deprecation, so minor fixes may not be worth implementing since the file will be replaced.
Applied to files:
lib/neo4j/repositories/workflowRun.ts__tests__/shared/adapters/neo4j/StorageAdapter.neo4j.test.tsshared/src/adapters/neo4j/queries/workflowRuns/addEvent.mapper.tslib/neo4j/services/workflow.tsshared/src/adapters/neo4j/queries/workflowRuns/addEvent.tsshared/src/adapters/neo4j/queries/workflowRuns/listEvents.mapper.tsshared/src/adapters/neo4j/queries/workflowRuns/listForIssue.mapper.tsshared/src/adapters/neo4j/queries/workflowRuns/listEvents.ts__tests__/shared/adapters/neo4j/queries.neo4j.test.tsshared/src/lib/neo4j/services/workflow.tsshared/src/adapters/neo4j/types.ts
📚 Learning: 2025-09-12T01:44:51.070Z
Learnt from: youngchingjui
Repo: youngchingjui/issue-to-pr PR: 1241
File: app/api/webhook/github/route.ts:85-90
Timestamp: 2025-09-12T01:44:51.070Z
Learning: In the webhook handlers at app/api/webhook/github/route.ts, runWithInstallationId is intentionally not awaited to allow fire-and-forget background processing of long-running workflows, avoiding webhook timeouts. The team plans to migrate these to proper queues/workers in a separate issue later.
Applied to files:
__tests__/api/webhook/github/route.test.tsshared/src/ports/db/index.tsapps/workers/workflow-workers/src/orchestrators/autoResolveIssue.tsshared/src/usecases/workflows/autoResolveIssue.ts
📚 Learning: 2026-01-02T09:20:57.476Z
Learnt from: youngchingjui
Repo: youngchingjui/issue-to-pr PR: 1450
File: shared/src/adapters/neo4j/types.ts:272-283
Timestamp: 2026-01-02T09:20:57.476Z
Learning: The plan schema and llmResponseWithPlanSchema in shared/src/adapters/neo4j/types.ts are being migrated away/deprecated, so schema inconsistencies related to plan support do not need to be fixed.
Applied to files:
__tests__/shared/adapters/neo4j/StorageAdapter.neo4j.test.tsshared/src/adapters/neo4j/queries/workflowRuns/addEvent.mapper.tsshared/src/adapters/neo4j/queries/workflowRuns/listEvents.mapper.tsshared/src/adapters/neo4j/queries/workflowRuns/listByUser.mapper.tsshared/src/adapters/neo4j/queries/workflowRuns/listForIssue.mapper.tsshared/src/adapters/neo4j/queries/workflowRuns/listForIssue.tsshared/src/adapters/neo4j/queries/workflowRuns/listEvents.tsshared/src/adapters/neo4j/StorageAdapter.ts__tests__/shared/adapters/neo4j/queries.neo4j.test.tsshared/src/adapters/neo4j/types.tsshared/src/adapters/neo4j/queries/workflowRuns/listForRepo.mapper.ts
📚 Learning: 2025-09-22T09:32:07.272Z
Learnt from: youngchingjui
Repo: youngchingjui/issue-to-pr PR: 1282
File: shared/src/entities/events/WorkflowEvent.ts:15-20
Timestamp: 2025-09-22T09:32:07.272Z
Learning: For workflow.started events in shared/src/entities/events/WorkflowEvent.ts, the content field is intentionally optional, not required. The schema correctly uses z.string().optional().
Applied to files:
shared/src/adapters/neo4j/queries/workflowRuns/addEvent.tsshared/src/adapters/neo4j/types.ts
📚 Learning: 2025-09-04T14:58:22.961Z
Learnt from: CR
Repo: youngchingjui/issue-to-pr PR: 0
File: .cursor/rules/env-management.mdc:0-0
Timestamp: 2025-09-04T14:58:22.961Z
Learning: Applies to .env : Never commit sensitive environment variables to the repository
Applied to files:
.gitignore
📚 Learning: 2025-09-10T06:22:04.642Z
Learnt from: youngchingjui
Repo: youngchingjui/issue-to-pr PR: 1223
File: components/RepositoryList.tsx:0-0
Timestamp: 2025-09-10T06:22:04.642Z
Learning: Repository links in RepositoryList.tsx should use repo.full_name to construct paths (e.g., `/${repo.full_name}/issues`) instead of manually combining owner and repository name, as this ensures correct routing for both personal and organization repositories.
Applied to files:
lib/webhook/github/types.ts
📚 Learning: 2025-09-11T09:11:14.319Z
Learnt from: youngchingjui
Repo: youngchingjui/issue-to-pr PR: 1214
File: lib/webhook.ts:184-189
Timestamp: 2025-09-11T09:11:14.319Z
Learning: The autoResolveIssue workflow in the codebase follows clean architecture principles with a two-argument signature: autoResolveIssue(params, ports), where ports contains auth and settings adapters.
Applied to files:
apps/workers/workflow-workers/src/orchestrators/autoResolveIssue.tslib/workflows/resolveIssue.tsshared/src/usecases/workflows/autoResolveIssue.ts
📚 Learning: 2025-09-11T09:11:14.319Z
Learnt from: youngchingjui
Repo: youngchingjui/issue-to-pr PR: 1214
File: lib/webhook.ts:184-189
Timestamp: 2025-09-11T09:11:14.319Z
Learning: The autoResolveIssue workflow signature is autoResolveIssue(params, ports) where params contains {issue, repository, jobId} and ports contains {auth: AuthReaderPort, settings: SettingsReaderPort, eventBus?: EventBusPort}.
Applied to files:
lib/workflows/resolveIssue.tsshared/src/usecases/workflows/autoResolveIssue.ts
🧬 Code graph analysis (13)
__tests__/shared/adapters/neo4j/workflowRuns.neo4j.test.ts (2)
__tests__/shared/adapters/neo4j/testUtils.ts (2)
createTestDataSource(23-41)verifyConnection(46-57)shared/src/adapters/neo4j/StorageAdapter.ts (1)
StorageAdapter(383-436)
shared/src/entities/WorkflowRun.ts (1)
shared/src/adapters/neo4j/types.ts (1)
WorkflowRun(322-322)
shared/src/adapters/neo4j/queries/workflowRuns/getWorkflowRunById.ts (1)
shared/src/adapters/neo4j/types.ts (6)
WorkflowRun(322-322)Repository(309-309)Issue(304-304)Commit(299-299)GithubWebhookEvent(303-303)GithubUser(302-302)
__tests__/api/webhook/github/route.test.ts (6)
lib/webhook/github/handlers/issue/label.autoResolveIssue.handler.ts (1)
handleIssueLabelAutoResolve(10-46)lib/webhook/github/handlers/issue/label.resolve.handler.ts (1)
handleIssueLabelResolve(22-89)lib/webhook/github/handlers/pullRequest/label.createDependentPR.handler.ts (1)
handlePullRequestLabelCreateDependentPR(8-40)lib/webhook/github/handlers/pullRequest/closed.removeContainer.handler.ts (1)
handlePullRequestClosedRemoveContainer(8-52)lib/webhook/github/handlers/installation/revalidateRepositoriesCache.handler.ts (1)
revalidateUserInstallationReposCache(12-17)lib/webhook/github/handlers/repository/edited.revalidateRepoCache.handler.ts (1)
handleRepositoryEditedRevalidate(9-17)
shared/src/adapters/neo4j/queries/workflowRuns/addEvent.mapper.ts (3)
shared/src/adapters/neo4j/queries/workflowRuns/addEvent.ts (1)
AddEventResult(43-48)shared/src/entities/events/index.ts (1)
AllEvents(6-6)shared/src/adapters/neo4j/types.ts (2)
anyEventSchema(284-295)AnyEvent(298-298)
shared/src/adapters/neo4j/queries/workflowRuns/getWorkflowRunById.mapper.ts (3)
shared/src/entities/WorkflowRun.ts (1)
WorkflowRunActor(17-17)shared/src/adapters/neo4j/types.ts (9)
githubUserSchema(46-52)githubWebhookEventSchema(98-101)genericWebhookEventSchema(105-111)userSchema(39-43)WorkflowRun(322-322)workflowRunSchema(30-36)repositorySchema(117-129)issueSchema(160-170)commitSchema(136-158)shared/src/adapters/neo4j/queries/workflowRuns/getWorkflowRunById.ts (1)
GetWorkflowRunByIdResult(36-44)
shared/src/adapters/neo4j/queries/workflowRuns/createWorkflowRun.mapper.ts (2)
shared/src/adapters/neo4j/queries/workflowRuns/createWorkflowRun.ts (2)
CreateWorkflowRunResult(38-40)CreateWorkflowRunParams(32-36)shared/src/adapters/neo4j/types.ts (2)
WorkflowRun(322-322)workflowRunSchema(30-36)
shared/src/ports/db/index.ts (3)
shared/src/entities/WorkflowRun.ts (3)
WorkflowRunTypes(19-25)WorkflowRunActor(17-17)WorkflowRun(27-37)shared/src/entities/events/index.ts (1)
AllEvents(6-6)shared/src/adapters/neo4j/types.ts (1)
WorkflowRun(322-322)
shared/src/adapters/neo4j/StorageAdapter.ts (6)
shared/src/ports/db/index.ts (9)
CreateWorkflowRunInput(27-34)WorkflowEventInput(36-40)Target(8-21)RepositoryAttachment(42-49)IssueAttachment(51-54)CommitAttachment(56-60)WorkflowRunHandle(62-74)DatabaseStorage(82-93)WorkflowRunFilter(76-80)shared/src/adapters/neo4j/queries/workflowRuns/createWorkflowRun.ts (2)
CreateWorkflowRunParams(32-36)createWorkflowRun(46-55)shared/src/adapters/neo4j/queries/workflowRuns/attachRepository.ts (1)
attachRepository(39-60)shared/src/adapters/neo4j/queries/workflowRuns/attachIssue.ts (1)
attachIssue(27-36)shared/src/adapters/neo4j/queries/workflowRuns/attachCommit.ts (1)
attachCommit(36-46)shared/src/adapters/neo4j/queries/workflowRuns/getWorkflowRunById.mapper.ts (1)
mapGetWorkflowRunById(81-144)
apps/workers/workflow-workers/src/orchestrators/autoResolveIssue.ts (4)
apps/workers/workflow-workers/src/helper.ts (1)
getEnvVar(55-58)shared/src/adapters/neo4j/repositories/SettingsReaderAdapter.ts (1)
makeSettingsReaderAdapter(28-54)shared/src/auth.ts (1)
setAccessToken(11-13)shared/src/adapters/neo4j/StorageAdapter.ts (1)
StorageAdapter(383-436)
shared/src/adapters/neo4j/queries/workflowRuns/listForRepo.ts (1)
shared/src/adapters/neo4j/types.ts (3)
User(320-320)GithubWebhookEvent(303-303)GithubUser(302-302)
shared/src/usecases/workflows/autoResolveIssue.ts (4)
shared/src/ports/db/index.ts (1)
DatabaseStorage(82-93)shared/src/ports/events/eventBus.ts (1)
EventBusPort(8-13)apps/workers/workflow-workers/src/orchestrators/autoResolveIssue.ts (1)
autoResolveIssue(46-121)lib/workflows/autoResolveIssue.ts (1)
autoResolveIssue(42-225)
shared/src/adapters/neo4j/queries/workflowRuns/listForRepo.mapper.ts (2)
shared/src/entities/WorkflowRun.ts (1)
WorkflowRunActor(17-17)shared/src/adapters/neo4j/types.ts (3)
githubWebhookEventSchema(98-101)githubUserSchema(46-52)userSchema(39-43)
🪛 Biome (2.1.2)
shared/src/adapters/neo4j/queries/workflowRuns/addEvent.mapper.ts
[error] 150-151: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
Comment on lines
132
to
145
| describe("workflow.run.listEvents", () => { | ||
| it("should return empty array (placeholder implementation)", async () => { | ||
| const result = await adapter.workflow.run.listEvents("any-run-id") | ||
| const result = await adapter.workflow.events.list("any-run-id") | ||
| expect(result).toEqual([]) | ||
| }) | ||
|
|
||
| it("should accept any run id without error", async () => { | ||
| await expect(adapter.workflow.events.list("test-run-1")).resolves.toEqual( | ||
| [] | ||
| ) | ||
| await expect( | ||
| adapter.workflow.run.listEvents("test-run-1") | ||
| ).resolves.toEqual([]) | ||
| await expect( | ||
| adapter.workflow.run.listEvents("non-existent") | ||
| adapter.workflow.events.list("non-existent") | ||
| ).resolves.toEqual([]) | ||
| }) |
Contributor
There was a problem hiding this comment.
Describe block name doesn't match the API being tested.
The describe block is named "workflow.run.listEvents" but the tests now call adapter.workflow.events.list. Update the describe name to match the new API path.
Proposed fix
- describe("workflow.run.listEvents", () => {
+ describe("workflow.events.list", () => {
it("should return empty array (placeholder implementation)", async () => {
const result = await adapter.workflow.events.list("any-run-id")
expect(result).toEqual([])📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| describe("workflow.run.listEvents", () => { | |
| it("should return empty array (placeholder implementation)", async () => { | |
| const result = await adapter.workflow.run.listEvents("any-run-id") | |
| const result = await adapter.workflow.events.list("any-run-id") | |
| expect(result).toEqual([]) | |
| }) | |
| it("should accept any run id without error", async () => { | |
| await expect(adapter.workflow.events.list("test-run-1")).resolves.toEqual( | |
| [] | |
| ) | |
| await expect( | |
| adapter.workflow.run.listEvents("test-run-1") | |
| ).resolves.toEqual([]) | |
| await expect( | |
| adapter.workflow.run.listEvents("non-existent") | |
| adapter.workflow.events.list("non-existent") | |
| ).resolves.toEqual([]) | |
| }) | |
| describe("workflow.events.list", () => { | |
| it("should return empty array (placeholder implementation)", async () => { | |
| const result = await adapter.workflow.events.list("any-run-id") | |
| expect(result).toEqual([]) | |
| }) | |
| it("should accept any run id without error", async () => { | |
| await expect(adapter.workflow.events.list("test-run-1")).resolves.toEqual( | |
| [] | |
| ) | |
| await expect( | |
| adapter.workflow.events.list("non-existent") | |
| ).resolves.toEqual([]) | |
| }) |
🤖 Prompt for AI Agents
In @__tests__/shared/adapters/neo4j/StorageAdapter.neo4j.test.ts around lines
132 - 145, The describe block name is outdated ("workflow.run.listEvents") and
should match the API under test; update the describe string to
"workflow.events.list" so it reflects the calls to adapter.workflow.events.list
in the tests (locate the describe that wraps the two it(...) cases and change
its title to "workflow.events.list").
Comment on lines
+29
to
+46
| // Hardcoded test data IDs for idempotent testing | ||
| const TEST_DATA = { | ||
| workflowRuns: [ | ||
| "test-workflow-run-minimal", | ||
| "test-workflow-run-full", | ||
| "test-workflow-run-events", | ||
| "test-workflow-run-step-by-step", | ||
| "test-workflow-run-backward-compat", | ||
| ], | ||
| repositories: ["test-prog-repo-1", "test-prog-repo-2"], | ||
| users: ["test-prog-user-1"], | ||
| githubUsers: ["test-prog-github-user-1"], | ||
| commits: ["test-prog-commit-1"], | ||
| issues: [ | ||
| { number: 42, repoFullName: "test/repo" }, | ||
| { number: 100, repoFullName: "test/prog-repo-2" }, | ||
| ], | ||
| } |
Contributor
There was a problem hiding this comment.
Fix test DB cleanup: repository deletion key doesn’t match what the tests create.
cleanupHardcodedTestData() deletes Repository by repo.id IN $repoIds (Line 88), but the tests create repos with id: 12345/67890 and fullName: test/prog-repo-*. TEST_DATA.repositories contains "test-prog-repo-1" etc., so repos likely won’t be deleted.
Proposed fix
- repositories: ["test-prog-repo-1", "test-prog-repo-2"],
+ repositories: ["test/prog-repo-1", "test/prog-repo-2"],- // Delete repositories
+ // Delete repositories (keyed by fullName, not id)
await session.run(
`
MATCH (repo:Repository)
- WHERE repo.id IN $repoIds
+ WHERE repo.fullName IN $repoFullNames
DETACH DELETE repo
`,
- { repoIds: TEST_DATA.repositories }
+ { repoFullNames: TEST_DATA.repositories }
)Also applies to: 84-92, 227-234, 280-287
🤖 Prompt for AI Agents
In @__tests__/shared/adapters/neo4j/workflowRuns.neo4j.test.ts around lines 29 -
46, The cleanup code is deleting Repository nodes by repo.id but the tests
create repos with fullName like "test/prog-repo-1", so change the cleanup to
delete by repo.fullName (e.g., MATCH (repo:Repository) WHERE repo.fullName IN
$repoFullNames DETACH DELETE repo) and pass TEST_DATA.repositories as the
$repoFullNames parameter, or alternatively update TEST_DATA.repositories to
contain the actual repo id values created by the tests; apply the same fix to
the other cleanup locations referenced (the other uses of repo.id IN $repoIds).
Comment on lines
+71
to
+146
| async function cleanupHardcodedTestData() { | ||
| const session = dataSource.getSession("WRITE") | ||
| try { | ||
| // Delete workflow runs | ||
| await session.run( | ||
| ` | ||
| MATCH (wr:WorkflowRun) | ||
| WHERE wr.id IN $runIds | ||
| DETACH DELETE wr | ||
| `, | ||
| { runIds: TEST_DATA.workflowRuns } | ||
| ) | ||
|
|
||
| // Delete repositories | ||
| await session.run( | ||
| ` | ||
| MATCH (repo:Repository) | ||
| WHERE repo.id IN $repoIds | ||
| DETACH DELETE repo | ||
| `, | ||
| { repoIds: TEST_DATA.repositories } | ||
| ) | ||
|
|
||
| // Delete users | ||
| await session.run( | ||
| ` | ||
| MATCH (user:User) | ||
| WHERE user.id IN $userIds | ||
| DETACH DELETE user | ||
| `, | ||
| { userIds: TEST_DATA.users } | ||
| ) | ||
|
|
||
| // Delete GitHub users | ||
| await session.run( | ||
| ` | ||
| MATCH (ghUser:GithubUser) | ||
| WHERE ghUser.id IN $ghUserIds | ||
| DETACH DELETE ghUser | ||
| `, | ||
| { ghUserIds: TEST_DATA.githubUsers } | ||
| ) | ||
|
|
||
| // Delete commits | ||
| await session.run( | ||
| ` | ||
| MATCH (commit:Commit) | ||
| WHERE commit.sha IN $commitShas | ||
| DETACH DELETE commit | ||
| `, | ||
| { commitShas: TEST_DATA.commits } | ||
| ) | ||
|
|
||
| // Delete issues | ||
| for (const issue of TEST_DATA.issues) { | ||
| await session.run( | ||
| ` | ||
| MATCH (issue:Issue {number: $number, repoFullName: $repoFullName}) | ||
| DETACH DELETE issue | ||
| `, | ||
| { number: int(issue.number), repoFullName: issue.repoFullName } | ||
| ) | ||
| } | ||
|
|
||
| // Delete events | ||
| await session.run( | ||
| ` | ||
| MATCH (event:Event) | ||
| WHERE event.id STARTS WITH 'test-prog-' | ||
| DETACH DELETE event | ||
| ` | ||
| ) | ||
| } finally { | ||
| await session.close() | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Prevent orphaned Event nodes: deleting only WorkflowRun leaves event chains behind.
DETACH DELETE wr removes relationships but not the connected Event nodes, so repeated runs will accumulate orphaned events. The current event cleanup (Line 136-142) relies on event.id STARTS WITH 'test-prog-', but handle.add.event() likely generates non-prefixed ids.
Proposed fix (delete run + attached events together)
- await session.run(
- `
- MATCH (wr:WorkflowRun {id: $id})
- DETACH DELETE wr
- `,
- { id: handle.run.id }
- )
+ await session.run(
+ `
+ MATCH (wr:WorkflowRun {id: $id})
+ OPTIONAL MATCH (wr)-[:STARTS_WITH|NEXT*]->(e:Event)
+ DETACH DELETE wr, e
+ `,
+ { id: handle.run.id }
+ )Apply similarly to the other per-test cleanups (Line 366-378, Line 398-409), and consider folding this into a shared helper to avoid drift.
Also applies to: 366-378, 398-409
Comment on lines
+148
to
+191
| describe("Minimal workflow run creation", () => { | ||
| it("should create workflow run with only type", async () => { | ||
| const handle = await adapter.workflow.run.create({ | ||
| id: TEST_DATA.workflowRuns[0], | ||
| type: "resolveIssue", | ||
| }) | ||
|
|
||
| expect(handle.run.id).toBe(TEST_DATA.workflowRuns[0]) | ||
| expect(handle.run.type).toBe("resolveIssue") | ||
| expect(handle.run.state).toBe("pending") | ||
|
|
||
| // Verify only WorkflowRun node exists, no relationships | ||
| const session = dataSource.getSession("READ") | ||
| try { | ||
| const result = await session.run( | ||
| ` | ||
| MATCH (wr:WorkflowRun {id: $id}) | ||
| RETURN | ||
| COUNT { (wr)-[:BASED_ON_REPOSITORY]->() } AS repoCount, | ||
| COUNT { (wr)-[:BASED_ON_ISSUE]->() } AS issueCount, | ||
| COUNT { (wr)-[:INITIATED_BY]->() } AS actorCount | ||
| `, | ||
| { id: handle.run.id } | ||
| ) | ||
|
|
||
| const record = result.records[0] | ||
| const repoCount = record?.get("repoCount") | ||
| const issueCount = record?.get("issueCount") | ||
| const actorCount = record?.get("actorCount") | ||
|
|
||
| // COUNT {} returns Neo4j Integer, convert to number | ||
| expect( | ||
| typeof repoCount === "number" ? repoCount : repoCount?.toNumber() | ||
| ).toBe(0) | ||
| expect( | ||
| typeof issueCount === "number" ? issueCount : issueCount?.toNumber() | ||
| ).toBe(0) | ||
| expect( | ||
| typeof actorCount === "number" ? actorCount : actorCount?.toNumber() | ||
| ).toBe(0) | ||
| } finally { | ||
| await session.close() | ||
| } | ||
| }) |
Contributor
There was a problem hiding this comment.
handle.run.state assertion may be incompatible with event-sourced state.
The test asserts handle.run.state === "pending" (Line 157). If state is intended to be derived from workflowState events (rather than stored on WorkflowRun), this assertion is brittle. Based on learnings, the WorkflowRun node intentionally does not include state.
Safer assertion options
- expect(handle.run.state).toBe("pending")
+ // If state is event-sourced, validate via events (or via getById if it computes state).
+ expect(handle.run.id).toBe(TEST_DATA.workflowRuns[0])Or, if getById() is the canonical computed view:
+ const retrieved = await adapter.workflow.run.getById(handle.run.id)
+ expect(retrieved?.state).toBe("pending")Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In @__tests__/shared/adapters/neo4j/workflowRuns.neo4j.test.ts around lines 148
- 191, The test asserts handle.run.state === "pending" even though
WorkflowRun.state is event-sourced and not stored on the node; remove or replace
that brittle assertion: either drop the state check entirely in the "Minimal
workflow run creation" test (the remaining id/type checks and relationship
counts suffice) or fetch the canonical computed view (e.g., via
adapter.workflow.run.getById or the event-sourced projection) and assert its
computed state there; update references to handle.run.state in this test
accordingly so you don't rely on a non-persisted field.
Comment on lines
+338
to
+379
| describe("Event chaining", () => { | ||
| it("should chain events in order via handle.add.event", async () => { | ||
| const handle = await adapter.workflow.run.create({ | ||
| type: "resolveIssue", | ||
| }) | ||
|
|
||
| // Add first event | ||
| await handle.add.event({ | ||
| type: "status", | ||
| payload: { content: "Event 1" }, | ||
| }) | ||
|
|
||
| // Add second event | ||
| await handle.add.event({ | ||
| type: "status", | ||
| payload: { content: "Event 2" }, | ||
| }) | ||
|
|
||
| // Add third event | ||
| await handle.add.event({ | ||
| type: "status", | ||
| payload: { content: "Event 3" }, | ||
| }) | ||
|
|
||
| // Verify events are in order | ||
| const events = await adapter.workflow.events.list(handle.run.id) | ||
| expect(events.length).toBeGreaterThanOrEqual(3) | ||
|
|
||
| // Cleanup | ||
| const session = dataSource.getSession("WRITE") | ||
| try { | ||
| await session.run( | ||
| ` | ||
| MATCH (wr:WorkflowRun {id: $id}) | ||
| DETACH DELETE wr | ||
| `, | ||
| { id: handle.run.id } | ||
| ) | ||
| } finally { | ||
| await session.close() | ||
| } | ||
| }) |
Contributor
There was a problem hiding this comment.
Event chaining test doesn’t verify ordering.
The test name says “in order” but only asserts events.length >= 3 (Line 364-365). That can pass even if ordering is wrong.
Suggested assertion
const events = await adapter.workflow.events.list(handle.run.id)
- expect(events.length).toBeGreaterThanOrEqual(3)
+ const statusEvents = events.filter((e) => e.type === "status")
+ expect(statusEvents.slice(-3).map((e) => (e as any).content)).toEqual([
+ "Event 1",
+ "Event 2",
+ "Event 3",
+ ])🤖 Prompt for AI Agents
In @__tests__/shared/adapters/neo4j/workflowRuns.neo4j.test.ts around lines 338
- 379, The test currently only checks events.length but not order; update the
assertion after calling adapter.workflow.events.list(handle.run.id) to verify
sequence by inspecting the returned events array (from
adapter.workflow.events.list) for ordering—e.g., map to event payload content or
order/timestamp fields and assert that the sequence equals ["Event 1","Event
2","Event 3"] (or assert events[0].payload.content === "Event 1",
events[1].payload.content === "Event 2", events[2].payload.content === "Event
3"). Ensure you reference the returned events from
adapter.workflow.events.list(handle.run.id) and keep the cleanup block
unchanged.
Comment on lines
+15
to
+21
| export function mapCreateWorkflowRunResult( | ||
| result: QueryResult<CreateWorkflowRunResult>, | ||
| params: CreateWorkflowRunParams | ||
| ): WorkflowRun { | ||
| const record = result.records[0] | ||
| const wrNode = record.get("wr") | ||
| const wr = workflowRunSchema.parse(wrNode.properties) |
Contributor
There was a problem hiding this comment.
Missing null check and unused parameter.
Two issues:
paramsis declared but never used. Consider removing it or documenting its intended future use.result.records[0]is accessed without a null check. If the query returns no records (e.g., due to a constraint violation), this will throw an obscure error.
Proposed fix
export function mapCreateWorkflowRunResult(
result: QueryResult<CreateWorkflowRunResult>,
- params: CreateWorkflowRunParams
+ _params: CreateWorkflowRunParams // Reserved for future use
): WorkflowRun {
const record = result.records[0]
+ if (!record) {
+ throw new Error("Failed to create workflow run: no record returned")
+ }
const wrNode = record.get("wr")
const wr = workflowRunSchema.parse(wrNode.properties)🤖 Prompt for AI Agents
In @shared/src/adapters/neo4j/queries/workflowRuns/createWorkflowRun.mapper.ts
around lines 15 - 21, The mapCreateWorkflowRunResult function currently declares
params but never uses it and accesses result.records[0] without checking for an
empty result; remove the unused params parameter (or document its intended
future use) and add a defensive null/length check on result.records before
accessing index 0 (e.g., ensure a record exists and throw or return a clear
error if not), then safely obtain wrNode from the validated record and call
workflowRunSchema.parse on its properties; ensure any thrown error includes
context about the createWorkflowRun query for easier debugging.
Comment on lines
+15
to
+23
| const QUERY = ` | ||
| CREATE (wr:WorkflowRun { | ||
| id: $runId, | ||
| type: $type, | ||
| postToGithub: $postToGithub, | ||
| createdAt: datetime() | ||
| }) | ||
| RETURN wr | ||
| ` |
Contributor
There was a problem hiding this comment.
Consider idempotency for workflow runs (CREATE → MERGE by id).
If runCreate can be retried (or if a duplicate webhook delivery happens), CREATE can introduce duplicate WorkflowRun nodes unless the DB enforces uniqueness on WorkflowRun.id.
Proposed fix
const QUERY = `
- CREATE (wr:WorkflowRun {
- id: $runId,
- type: $type,
- postToGithub: $postToGithub,
- createdAt: datetime()
- })
+ MERGE (wr:WorkflowRun { id: $runId })
+ ON CREATE SET
+ wr.type = $type,
+ wr.postToGithub = $postToGithub,
+ wr.createdAt = datetime()
+ ON MATCH SET
+ wr.type = $type,
+ wr.postToGithub = $postToGithub
RETURN wr
`Also applies to: 46-55
🤖 Prompt for AI Agents
In @shared/src/adapters/neo4j/queries/workflowRuns/createWorkflowRun.ts around
lines 15 - 23, The CREATE of WorkflowRun is not idempotent; change the Cypher in
QUERY (and the other occurrence around the second block) to MERGE on wr by id
($runId) and use ON CREATE SET to assign properties like type, postToGithub, and
createdAt so they are set only when a new node is created, and use ON MATCH SET
if you want to update mutable fields on duplicate deliveries; update the query
constants referenced (e.g., QUERY and the similar query at lines 46-55)
accordingly.
Comment on lines
+26
to
+76
| function mapActor( | ||
| actorNode: Node | null, | ||
| webhookEventNode: Node | null, | ||
| senderNode: Node | null, | ||
| installationId: string | ||
| ): WorkflowRunActor | undefined { | ||
| // Webhook-triggered workflow | ||
| if (webhookEventNode && senderNode) { | ||
| const githubUser = githubUserSchema.parse(senderNode.properties) | ||
|
|
||
| // Try parsing with specific schema first, fall back to generic schema | ||
| let webhookEvent | ||
| const parseResult = githubWebhookEventSchema.safeParse( | ||
| webhookEventNode.properties | ||
| ) | ||
| if (parseResult.success) { | ||
| webhookEvent = parseResult.data | ||
| } else { | ||
| // Fall back to generic schema for non-standard webhook events | ||
| webhookEvent = genericWebhookEventSchema.parse( | ||
| webhookEventNode.properties | ||
| ) | ||
| } | ||
|
|
||
| return { | ||
| type: "webhook", | ||
| source: "github", | ||
| event: webhookEvent.event, | ||
| action: webhookEvent.action ?? "", | ||
| sender: { | ||
| id: githubUser.id, | ||
| login: githubUser.login, | ||
| }, | ||
| installationId, | ||
| } | ||
| } | ||
|
|
||
| // User-initiated workflow | ||
| if (actorNode) { | ||
| const labels = Array.from(actorNode.labels || []) as string[] | ||
| if (labels.includes("User")) { | ||
| const user = userSchema.parse(actorNode.properties) | ||
| return { | ||
| type: "user", | ||
| userId: user.id, | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return undefined | ||
| } |
Contributor
There was a problem hiding this comment.
Prefer omitting webhook action rather than forcing empty string.
action: webhookEvent.action ?? "" (Line 54-55) can introduce meaningless empty actions. If the domain type allows optional action, pass undefined instead.
Proposed fix
- action: webhookEvent.action ?? "",
+ ...(webhookEvent.action ? { action: webhookEvent.action } : {}),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| function mapActor( | |
| actorNode: Node | null, | |
| webhookEventNode: Node | null, | |
| senderNode: Node | null, | |
| installationId: string | |
| ): WorkflowRunActor | undefined { | |
| // Webhook-triggered workflow | |
| if (webhookEventNode && senderNode) { | |
| const githubUser = githubUserSchema.parse(senderNode.properties) | |
| // Try parsing with specific schema first, fall back to generic schema | |
| let webhookEvent | |
| const parseResult = githubWebhookEventSchema.safeParse( | |
| webhookEventNode.properties | |
| ) | |
| if (parseResult.success) { | |
| webhookEvent = parseResult.data | |
| } else { | |
| // Fall back to generic schema for non-standard webhook events | |
| webhookEvent = genericWebhookEventSchema.parse( | |
| webhookEventNode.properties | |
| ) | |
| } | |
| return { | |
| type: "webhook", | |
| source: "github", | |
| event: webhookEvent.event, | |
| action: webhookEvent.action ?? "", | |
| sender: { | |
| id: githubUser.id, | |
| login: githubUser.login, | |
| }, | |
| installationId, | |
| } | |
| } | |
| // User-initiated workflow | |
| if (actorNode) { | |
| const labels = Array.from(actorNode.labels || []) as string[] | |
| if (labels.includes("User")) { | |
| const user = userSchema.parse(actorNode.properties) | |
| return { | |
| type: "user", | |
| userId: user.id, | |
| } | |
| } | |
| } | |
| return undefined | |
| } | |
| function mapActor( | |
| actorNode: Node | null, | |
| webhookEventNode: Node | null, | |
| senderNode: Node | null, | |
| installationId: string | |
| ): WorkflowRunActor | undefined { | |
| // Webhook-triggered workflow | |
| if (webhookEventNode && senderNode) { | |
| const githubUser = githubUserSchema.parse(senderNode.properties) | |
| // Try parsing with specific schema first, fall back to generic schema | |
| let webhookEvent | |
| const parseResult = githubWebhookEventSchema.safeParse( | |
| webhookEventNode.properties | |
| ) | |
| if (parseResult.success) { | |
| webhookEvent = parseResult.data | |
| } else { | |
| // Fall back to generic schema for non-standard webhook events | |
| webhookEvent = genericWebhookEventSchema.parse( | |
| webhookEventNode.properties | |
| ) | |
| } | |
| return { | |
| type: "webhook", | |
| source: "github", | |
| event: webhookEvent.event, | |
| ...(webhookEvent.action ? { action: webhookEvent.action } : {}), | |
| sender: { | |
| id: githubUser.id, | |
| login: githubUser.login, | |
| }, | |
| installationId, | |
| } | |
| } | |
| // User-initiated workflow | |
| if (actorNode) { | |
| const labels = Array.from(actorNode.labels || []) as string[] | |
| if (labels.includes("User")) { | |
| const user = userSchema.parse(actorNode.properties) | |
| return { | |
| type: "user", | |
| userId: user.id, | |
| } | |
| } | |
| } | |
| return undefined | |
| } |
🤖 Prompt for AI Agents
In @shared/src/adapters/neo4j/queries/workflowRuns/getWorkflowRunById.mapper.ts
around lines 26 - 76, The mapActor function should not coerce missing
webhookEvent.action to an empty string; update the webhook branch in mapActor to
set action to webhookEvent.action (which will be undefined when absent) instead
of using `?? ""`, and ensure the WorkflowRunActor type/interface accepts an
optional action so no runtime or type errors occur.
Comment on lines
+95
to
+143
| // Validate and parse WorkflowRun | ||
| const wr = workflowRunSchema.parse(wrNode.properties) | ||
|
|
||
| // Parse repository to get installationId | ||
| const repo = repoNode ? repositorySchema.parse(repoNode.properties) : null | ||
| const installationId = repo?.githubInstallationId ?? "" | ||
|
|
||
| // Map optional relationships | ||
| const actor = mapActor( | ||
| actorNode, | ||
| webhookEventNode, | ||
| senderNode, | ||
| installationId | ||
| ) | ||
| const repository = repo ? { fullName: repo.fullName } : undefined | ||
| const issue = issueNode | ||
| ? (() => { | ||
| const parsed = issueSchema.parse(issueNode.properties) | ||
| return { | ||
| repoFullName: parsed.repoFullName, | ||
| number: | ||
| typeof parsed.number === "number" | ||
| ? parsed.number | ||
| : parsed.number.toNumber(), | ||
| } | ||
| })() | ||
| : undefined | ||
| const commit = commitNode | ||
| ? (() => { | ||
| const parsed = commitSchema.parse(commitNode.properties) | ||
| return { | ||
| sha: parsed.sha, | ||
| message: parsed.message, | ||
| repository: repository!, | ||
| } | ||
| })() | ||
| : undefined | ||
|
|
||
| return { | ||
| id: wr.id, | ||
| type: wr.type, | ||
| createdAt: wr.createdAt.toStandardDate(), | ||
| postToGithub: wr.postToGithub ?? false, | ||
| state: wr.state ?? "pending", | ||
| actor, | ||
| repository, | ||
| issue, | ||
| commit, | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find workflowRunSchema definition and related files
fd -e ts "workflowRun" shared/src/adapters/neo4j/ | head -20Repository: youngchingjui/issue-to-pr
Length of output: 51
🏁 Script executed:
#!/bin/bash
# Check workflowRunSchema definition
rg -n "workflowRunSchema|WorkflowRunSchema" shared/src/adapters/neo4j/ -A 3 -B 1Repository: youngchingjui/issue-to-pr
Length of output: 6199
🏁 Script executed:
#!/bin/bash
# Look at the getWorkflowRunById query file to see what it fetches
cat shared/src/adapters/neo4j/queries/workflowRuns/getWorkflowRunById.ts 2>/dev/null || echo "File not found, searching for similar name"
fd "getWorkflowRunById" shared/src/adapters/neo4j/queries/Repository: youngchingjui/issue-to-pr
Length of output: 1746
🏁 Script executed:
#!/bin/bash
# Check how listForRepo derives state from workflowState events
rg -n "listForRepo|workflowState" shared/src/adapters/neo4j/queries/workflowRuns/ -A 5 -B 2Repository: youngchingjui/issue-to-pr
Length of output: 6520
🏁 Script executed:
#!/bin/bash
# Get complete workflowRunSchema definition
sed -n '30,50p' shared/src/adapters/neo4j/types.tsRepository: youngchingjui/issue-to-pr
Length of output: 711
🏁 Script executed:
#!/bin/bash
# Also check if there's a workflowRunStateSchema to understand the distinction
rg -n "workflowRunStateSchema\s*=" shared/src/adapters/neo4j/types.ts -A 5Repository: youngchingjui/issue-to-pr
Length of output: 199
State must be derived from workflowState events, not from the WorkflowRun node.
Line 138 uses state: wr.state ?? "pending", but the WorkflowRun node doesn't store state. Other query paths (listForRepo, listForIssue, listByUser) correctly derive state from the latest workflowState Event by matching (w)-[:STARTS_WITH|NEXT*]->(e:Event {type: 'workflowState'}) and using latestWorkflowState.state. The getWorkflowRunById query doesn't fetch workflowState events, so the mapper cannot correctly determine state and will always default to "pending".
Update the getWorkflowRunById query to fetch the latest workflowState event and pass it to the mapper, then use that event's state instead of the node property.
🤖 Prompt for AI Agents
In @shared/src/adapters/neo4j/queries/workflowRuns/getWorkflowRunById.mapper.ts
around lines 95 - 143, The mapper is incorrectly deriving state from wr.state
(workflowRun node) which is not stored there; update the getWorkflowRunById
Cypher to also MATCH the latest workflowState Event (same pattern used in
listForRepo/listForIssue/listByUser: `(w)-[:STARTS_WITH|NEXT*]->(e:Event {type:
'workflowState'})` and return it as latestWorkflowState) and pass that node into
this mapper; then change the mapping logic to use
latestWorkflowState.properties.state (or its parsed event via the event schema)
instead of wr.state (so replace `state: wr.state ?? "pending"` with the derived
`state` from latestWorkflowState, falling back to "pending" if missing). Ensure
the mapper references the returned latestWorkflowState symbol and parses it
consistently with other mappers.
Comment on lines
+98
to
+131
| // Parse repository to get installationId | ||
| const repo = repoNode ? repositorySchema.parse(repoNode.properties) : null | ||
| const installationId = repo?.githubInstallationId ?? "" | ||
|
|
||
| // Map optional relationships | ||
| const actor = mapActor( | ||
| actorNode, | ||
| webhookEventNode, | ||
| senderNode, | ||
| installationId | ||
| ) | ||
| const repository = repo ? { fullName: repo.fullName } : undefined | ||
| const issue = issueNode | ||
| ? (() => { | ||
| const parsed = issueSchema.parse(issueNode.properties) | ||
| return { | ||
| repoFullName: parsed.repoFullName, | ||
| number: | ||
| typeof parsed.number === "number" | ||
| ? parsed.number | ||
| : parsed.number.toNumber(), | ||
| } | ||
| })() | ||
| : undefined | ||
| const commit = commitNode | ||
| ? (() => { | ||
| const parsed = commitSchema.parse(commitNode.properties) | ||
| return { | ||
| sha: parsed.sha, | ||
| message: parsed.message, | ||
| repository: repository!, | ||
| } | ||
| })() | ||
| : undefined |
Contributor
There was a problem hiding this comment.
Crash risk: repository! can throw when commit exists but repo is null.
commit mapping sets repository: repository! (Line 128-129). If a run has a commit attached but repository isn’t attached/returned, this will throw at runtime.
Proposed fix (safe commit mapping)
- const commit = commitNode
- ? (() => {
- const parsed = commitSchema.parse(commitNode.properties)
- return {
- sha: parsed.sha,
- message: parsed.message,
- repository: repository!,
- }
- })()
- : undefined
+ const commit = commitNode
+ ? (() => {
+ const parsed = commitSchema.parse(commitNode.properties)
+ if (!repository) return undefined
+ return {
+ sha: parsed.sha,
+ message: parsed.message,
+ repository,
+ }
+ })()
+ : undefinedIf the domain requires commit.repository always, the better fix is to update getWorkflowRunById to also fetch (commit)-[:IN_REPOSITORY]->(repo) when wr-[:BASED_ON_REPOSITORY]->(repo) is missing.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Parse repository to get installationId | |
| const repo = repoNode ? repositorySchema.parse(repoNode.properties) : null | |
| const installationId = repo?.githubInstallationId ?? "" | |
| // Map optional relationships | |
| const actor = mapActor( | |
| actorNode, | |
| webhookEventNode, | |
| senderNode, | |
| installationId | |
| ) | |
| const repository = repo ? { fullName: repo.fullName } : undefined | |
| const issue = issueNode | |
| ? (() => { | |
| const parsed = issueSchema.parse(issueNode.properties) | |
| return { | |
| repoFullName: parsed.repoFullName, | |
| number: | |
| typeof parsed.number === "number" | |
| ? parsed.number | |
| : parsed.number.toNumber(), | |
| } | |
| })() | |
| : undefined | |
| const commit = commitNode | |
| ? (() => { | |
| const parsed = commitSchema.parse(commitNode.properties) | |
| return { | |
| sha: parsed.sha, | |
| message: parsed.message, | |
| repository: repository!, | |
| } | |
| })() | |
| : undefined | |
| // Parse repository to get installationId | |
| const repo = repoNode ? repositorySchema.parse(repoNode.properties) : null | |
| const installationId = repo?.githubInstallationId ?? "" | |
| // Map optional relationships | |
| const actor = mapActor( | |
| actorNode, | |
| webhookEventNode, | |
| senderNode, | |
| installationId | |
| ) | |
| const repository = repo ? { fullName: repo.fullName } : undefined | |
| const issue = issueNode | |
| ? (() => { | |
| const parsed = issueSchema.parse(issueNode.properties) | |
| return { | |
| repoFullName: parsed.repoFullName, | |
| number: | |
| typeof parsed.number === "number" | |
| ? parsed.number | |
| : parsed.number.toNumber(), | |
| } | |
| })() | |
| : undefined | |
| const commit = commitNode | |
| ? (() => { | |
| const parsed = commitSchema.parse(commitNode.properties) | |
| if (!repository) return undefined | |
| return { | |
| sha: parsed.sha, | |
| message: parsed.message, | |
| repository, | |
| } | |
| })() | |
| : undefined |
🤖 Prompt for AI Agents
In @shared/src/adapters/neo4j/queries/workflowRuns/getWorkflowRunById.mapper.ts
around lines 98 - 131, The commit mapping uses a non-null assertion repository!
which will throw if a commitNode exists but repoNode is missing; update the
commit mapping in getWorkflowRunById mapper to avoid the non-null assertion by
making commit.repository optional or deriving a repository value from the commit
properties (e.g., use parsed.repository/repoFullName from commitSchema if
present) and only set repository when you have valid data, or alternatively
ensure upstream query always returns the repo when a commit exists (fetch
(commit)-[:IN_REPOSITORY]->(repo)) if domain requires a repository.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Context
This PR is the second step after introducing the StorageAdapter infra. It updates workflow creation paths to actually use the StorageAdapter and persist Repository, User/GithubUser, and attribution data for new workflow runs. It also enhances the GitHub webhook types to carry full repository metadata.
Summary of changes
Webhook payloads
resolveIssue workflow
Webhook handlers
Workers orchestrator
Other action files
Cleanup deprecated functions
Notes and considerations
Why this approach
Testing
Follow-ups
Closes: Part of the epic to migrate WorkflowRun data model; depends on StorageAdapter infra from previous PR.
Closes #1446
Update (follow-up after review)
Addressed feedback from @youngchingjui:
Notes
Commit
Follow-up: migrate more workflows to StorageAdapter
What changed
Implementation notes
{ type: 'user', userId: 'system' }).Verification
Next steps
Addresses
Summary by CodeRabbit
Release Notes
New Features
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.