fix: ignore invalid run IDs in activity logging#798
Open
brew-agent wants to merge 2 commits intopaperclipai:masterfrom
Open
fix: ignore invalid run IDs in activity logging#798brew-agent wants to merge 2 commits intopaperclipai:masterfrom
brew-agent wants to merge 2 commits intopaperclipai:masterfrom
Conversation
Before inserting into activity_log, validate that the supplied run_id actually exists in heartbeat_runs. If the run is not found (e.g. a stale/nonexistent ID from the x-paperclip-run-id header), fall back to null so the insert succeeds instead of throwing a foreign-key violation 500. Affects all routes that call logActivity with actor.runId, including PATCH /api/issues/:id and many others. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
Greptile SummaryThis PR fixes FK constraint violations in
Confidence Score: 3/5
Important Files Changed
Prompt To Fix All With AIThis is a comment left during a code review.
Path: server/src/services/activity-log.ts
Line: 20-28
Comment:
**`resolveRunId` propagates DB errors, reintroducing 500s**
The PR's stated goal is to "gracefully null out stale or invalid run ids instead of throwing FK 500s," but `resolveRunId` has no error handling. If the `heartbeat_runs` query throws (e.g., connection timeout, table lock, transient network error), the exception propagates through `logActivity` and the caller still gets a 500 — now from the lookup rather than the insert.
Since the fallback behavior for an unresolvable `runId` is already `null`, wrapping the query in a try/catch and returning `null` on failure would fully honor the graceful-degradation intent:
```ts
async function resolveRunId(db: Db, runId: string | null | undefined): Promise<string | null> {
if (!runId) return null;
try {
const row = await db
.select({ id: heartbeatRuns.id })
.from(heartbeatRuns)
.where(eq(heartbeatRuns.id, runId))
.then((rows) => rows[0] ?? null);
return row ? runId : null;
} catch {
return null;
}
}
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: server/src/services/activity-log.ts
Line: 33
Comment:
**Extra DB round-trip on every `logActivity` call with a `runId`**
`resolveRunId` is now called unconditionally before every insert, meaning any caller that supplies a `runId` pays an additional SELECT against `heartbeat_runs`. If `logActivity` is invoked on every issue mutation (which the PR description implies), this doubles the DB traffic for those paths and adds latency in the hot path.
A few alternatives worth considering:
- Pass the validated `runId` in from the call-site (at the point where the run is started, the ID is already known to be valid).
- Cache the lookup for the duration of a request, since a single request is unlikely to reference more than one run.
- Accept the overhead if `logActivity` is low-frequency, but document the trade-off explicitly.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: server/src/__tests__/activity-log-invalid-run-id.test.ts
Line: 10-28
Comment:
**`inserted` array is collected but never asserted upon**
`createDbStub` populates an `inserted` array via the stub `values` mock, but all three tests assert only on `values.mock.calls`. The `inserted` return value is destructured in the first test but then unused — all tests end up going through `values.mock.calls[0][0]` directly. The `inserted` array and its population inside `values` can be removed to reduce noise, or assertions should be switched to use it consistently.
Also, no test covers the case where `runId` is `undefined` (i.e., the field is omitted from the input entirely). While `!runId` handles `undefined` the same as `null`, an explicit test for this path would document that contract and guard against future changes to the guard condition.
How can I resolve this? If you propose a fix, please make it concise.Last reviewed commit: 0a8a5c7 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+20
to
+28
| async function resolveRunId(db: Db, runId: string | null | undefined): Promise<string | null> { | ||
| if (!runId) return null; | ||
| const row = await db | ||
| .select({ id: heartbeatRuns.id }) | ||
| .from(heartbeatRuns) | ||
| .where(eq(heartbeatRuns.id, runId)) | ||
| .then((rows) => rows[0] ?? null); | ||
| return row ? runId : null; | ||
| } |
Contributor
There was a problem hiding this comment.
resolveRunId propagates DB errors, reintroducing 500s
The PR's stated goal is to "gracefully null out stale or invalid run ids instead of throwing FK 500s," but resolveRunId has no error handling. If the heartbeat_runs query throws (e.g., connection timeout, table lock, transient network error), the exception propagates through logActivity and the caller still gets a 500 — now from the lookup rather than the insert.
Since the fallback behavior for an unresolvable runId is already null, wrapping the query in a try/catch and returning null on failure would fully honor the graceful-degradation intent:
async function resolveRunId(db: Db, runId: string | null | undefined): Promise<string | null> {
if (!runId) return null;
try {
const row = await db
.select({ id: heartbeatRuns.id })
.from(heartbeatRuns)
.where(eq(heartbeatRuns.id, runId))
.then((rows) => rows[0] ?? null);
return row ? runId : null;
} catch {
return null;
}
}Prompt To Fix With AI
This is a comment left during a code review.
Path: server/src/services/activity-log.ts
Line: 20-28
Comment:
**`resolveRunId` propagates DB errors, reintroducing 500s**
The PR's stated goal is to "gracefully null out stale or invalid run ids instead of throwing FK 500s," but `resolveRunId` has no error handling. If the `heartbeat_runs` query throws (e.g., connection timeout, table lock, transient network error), the exception propagates through `logActivity` and the caller still gets a 500 — now from the lookup rather than the insert.
Since the fallback behavior for an unresolvable `runId` is already `null`, wrapping the query in a try/catch and returning `null` on failure would fully honor the graceful-degradation intent:
```ts
async function resolveRunId(db: Db, runId: string | null | undefined): Promise<string | null> {
if (!runId) return null;
try {
const row = await db
.select({ id: heartbeatRuns.id })
.from(heartbeatRuns)
.where(eq(heartbeatRuns.id, runId))
.then((rows) => rows[0] ?? null);
return row ? runId : null;
} catch {
return null;
}
}
```
How can I resolve this? If you propose a fix, please make it concise.| export async function logActivity(db: Db, input: LogActivityInput) { | ||
| const sanitizedDetails = input.details ? sanitizeRecord(input.details) : null; | ||
| const redactedDetails = sanitizedDetails ? redactCurrentUserValue(sanitizedDetails) : null; | ||
| const resolvedRunId = await resolveRunId(db, input.runId); |
Contributor
There was a problem hiding this comment.
Extra DB round-trip on every logActivity call with a runId
resolveRunId is now called unconditionally before every insert, meaning any caller that supplies a runId pays an additional SELECT against heartbeat_runs. If logActivity is invoked on every issue mutation (which the PR description implies), this doubles the DB traffic for those paths and adds latency in the hot path.
A few alternatives worth considering:
- Pass the validated
runIdin from the call-site (at the point where the run is started, the ID is already known to be valid). - Cache the lookup for the duration of a request, since a single request is unlikely to reference more than one run.
- Accept the overhead if
logActivityis low-frequency, but document the trade-off explicitly.
Prompt To Fix With AI
This is a comment left during a code review.
Path: server/src/services/activity-log.ts
Line: 33
Comment:
**Extra DB round-trip on every `logActivity` call with a `runId`**
`resolveRunId` is now called unconditionally before every insert, meaning any caller that supplies a `runId` pays an additional SELECT against `heartbeat_runs`. If `logActivity` is invoked on every issue mutation (which the PR description implies), this doubles the DB traffic for those paths and adds latency in the hot path.
A few alternatives worth considering:
- Pass the validated `runId` in from the call-site (at the point where the run is started, the ID is already known to be valid).
- Cache the lookup for the duration of a request, since a single request is unlikely to reference more than one run.
- Accept the overhead if `logActivity` is low-frequency, but document the trade-off explicitly.
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+10
to
+28
| function createDbStub(runExists: boolean) { | ||
| const inserted: unknown[] = []; | ||
|
|
||
| // Chain for select().from().where().then() | ||
| const thenFn = vi.fn((cb: (rows: unknown[]) => unknown) => | ||
| Promise.resolve(cb(runExists ? [{ id: "run-1" }] : [])), | ||
| ); | ||
| const where = vi.fn(() => ({ then: thenFn })); | ||
| const from = vi.fn(() => ({ where })); | ||
| const select = vi.fn(() => ({ from })); | ||
|
|
||
| // Chain for insert().values() | ||
| const values = vi.fn(async (row: unknown) => { | ||
| inserted.push(row); | ||
| }); | ||
| const insert = vi.fn(() => ({ values })); | ||
|
|
||
| return { db: { select, insert } as any, inserted, values }; | ||
| } |
Contributor
There was a problem hiding this comment.
inserted array is collected but never asserted upon
createDbStub populates an inserted array via the stub values mock, but all three tests assert only on values.mock.calls. The inserted return value is destructured in the first test but then unused — all tests end up going through values.mock.calls[0][0] directly. The inserted array and its population inside values can be removed to reduce noise, or assertions should be switched to use it consistently.
Also, no test covers the case where runId is undefined (i.e., the field is omitted from the input entirely). While !runId handles undefined the same as null, an explicit test for this path would document that contract and guard against future changes to the guard condition.
Prompt To Fix With AI
This is a comment left during a code review.
Path: server/src/__tests__/activity-log-invalid-run-id.test.ts
Line: 10-28
Comment:
**`inserted` array is collected but never asserted upon**
`createDbStub` populates an `inserted` array via the stub `values` mock, but all three tests assert only on `values.mock.calls`. The `inserted` return value is destructured in the first test but then unused — all tests end up going through `values.mock.calls[0][0]` directly. The `inserted` array and its population inside `values` can be removed to reduce noise, or assertions should be switched to use it consistently.
Also, no test covers the case where `runId` is `undefined` (i.e., the field is omitted from the input entirely). While `!runId` handles `undefined` the same as `null`, an explicit test for this path would document that contract and guard against future changes to the guard condition.
How can I resolve this? If you propose a fix, please make it concise.
Move the invalid-run-id guard out of logActivity and into actorMiddleware so downstream code only ever sees a verified run id: - board/unauthenticated actors never carry a runId (header is ignored) - agent actors (JWT and API key) validate the claimed runId against heartbeat_runs matching both agentId and companyId; best-effort: DB errors fall back to undefined instead of breaking the request - logActivity is simplified back to a straight insert (no lookup) Tests updated: old activity-log unit test replaced with a pass-through smoke test; new actor-middleware-run-id.test.ts covers all four resolution branches and the board-actor exclusion. Co-Authored-By: Paperclip <noreply@paperclip.ing>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
runIdvalues inactorMiddlewarebefore route code runsrunIdon agent actors when it matches a realheartbeat_runsrow for the same agent and companyrunIdfor board/session requests and degrade toundefinedon lookup errorslogActivitysimple again and add focused tests around actor run-id handlingWhy
Some API requests were carrying stale or fabricated run IDs via
x-paperclip-run-idor agent auth context. Those values flowed intoactivity_log.run_id, violated the FK toheartbeat_runs, and turned otherwise-successful mutations into 500 responses.This version fixes the problem at the request-auth boundary instead of inside
logActivity. That keeps invalid run IDs out of downstream route handling entirely, preserves valid run linkage, and avoids adding an extra lookup inside everylogActivitycall.