Skip to content

Commit c8ae355

Browse files
MathurAditya724MathurAditya724
committed
fix: resolve review findings — 16 bugs, DRY violations, and inconsistencies
Critical fixes: - bootstrap: resolve bot identity BEFORE creating handlers so ignore_authors self-loop guard actually works (was normalized with null botLogin, making $BOT_LOGIN substitution a no-op) - pipeline: remove leaked abort timer in stale-session retry that could fire prematurely before semaphore acquisition - migrations: use actual column type from EXPECTED_TABLES in repair instead of hardcoded TEXT (would corrupt INTEGER columns) - github-api: fix this-binding bug in fetchEntity that breaks when the fetcher is destructured; extract shared fetchByType helper Medium fixes: - bootstrap: add --global flag to git config calls (was writing to local repo config of whatever CWD happened to be) - auto-merge: use timeline API instead of issues events endpoint for ready_for_review (PR events don't appear in issue events) - fix-ci: filter attempt count by author to avoid miscounting human comments - pr: guard against empty checks array being misread as CI passed when CI hasn't started yet - respond-to-comment, review-pr: use $ME identity variable instead of redundant gh api user calls - mark-pr-ready: fix wording that contradicted auto-merge flow DRY consolidation: - github-api: extract shared fetchByType helper, eliminating duplicate try/catch/Sentry logging in fetchPR and fetchIssue - entity: consolidate 3 identical PR event handlers into one using a Set; consolidate check_suite/workflow_run into a shared lookup Documentation: - storage: document raw SQL vs Drizzle split and timestamp strategy - schema: document that Drizzle enum is TypeScript-only, SQL CHECK comes from migrations
1 parent c3ae429 commit c8ae355

13 files changed

Lines changed: 144 additions & 154 deletions

File tree

‎packages/opentower/src/bootstrap.ts‎

Lines changed: 33 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -57,12 +57,40 @@ export async function bootstrap(opts: BootstrapOptions): Promise<BootstrapResult
5757
return null
5858
}
5959

60-
const triggers = (cfg.triggers ?? []).map((t) => normalizeTrigger(t, null))
60+
// Resolve bot identity BEFORE normalizing triggers so that
61+
// ignore_authors substitution ($BOT_LOGIN) works correctly.
62+
const { auth } = createHandlers({ triggers: [], githubApp })
63+
let botLogin: string | null = null
64+
let tokenRefreshTimer: ReturnType<typeof setInterval> | null = null
65+
66+
try {
67+
const { botLogin: resolvedLogin, tokenRefreshTimer: refreshTimer } = await initAppIdentity(auth)
68+
botLogin = resolvedLogin
69+
tokenRefreshTimer = refreshTimer
70+
logger.info("GitHub App identity resolved", { login: botLogin })
71+
Sentry.setTag("bot.login", botLogin)
72+
} catch (_err) {
73+
botLogin = await resolveBotLogin()
74+
if (botLogin) {
75+
logger.warn("GitHub App identity failed, using GH_TOKEN identity", { login: botLogin })
76+
Sentry.setTag("bot.login", botLogin)
77+
} else {
78+
logger.warn("could not resolve bot identity -- self-loop guard degraded")
79+
}
80+
}
81+
82+
// Normalize triggers with the resolved bot login so ignore_authors
83+
// substitution works. This must happen after identity resolution.
84+
const triggers = (cfg.triggers ?? []).map((t) => normalizeTrigger(t, botLogin))
6185
if (triggers.length === 0) {
6286
logger.info("no triggers configured -- listener disabled", { path: configPath() })
6387
return null
6488
}
6589

90+
// Re-create handlers with properly normalized triggers (the initial
91+
// createHandlers call above was only to get the auth object).
92+
const { handlers: normalizedHandlers } = createHandlers({ triggers, githubApp })
93+
6694
const batchWindowMs = cfg.batch_window_ms ?? 5_000
6795
const dedup = makeDedup()
6896
const semaphore = makeSemaphore(maxConcurrent)
@@ -121,37 +149,6 @@ export async function bootstrap(opts: BootstrapOptions): Promise<BootstrapResult
121149
cronScheduler.start()
122150
logger.info("cron scheduler started")
123151

124-
const { handlers, auth } = createHandlers({
125-
triggers,
126-
githubApp,
127-
})
128-
129-
// Resolve bot identity from the GitHub App. The app's bot login is
130-
// "<slug>[bot]" and the installation token is set as GH_TOKEN so the
131-
// agent's gh CLI calls are attributed to the app.
132-
let botLogin: string | null = null
133-
let tokenRefreshTimer: ReturnType<typeof setInterval> | null = null
134-
135-
try {
136-
const { botLogin: resolvedLogin, tokenRefreshTimer: refreshTimer } = await initAppIdentity(auth)
137-
botLogin = resolvedLogin
138-
tokenRefreshTimer = refreshTimer
139-
logger.info("GitHub App identity resolved", { login: botLogin })
140-
Sentry.setTag("bot.login", botLogin)
141-
} catch (_err) {
142-
// Fall back to GH_TOKEN-based identity if App identity fails
143-
botLogin = await resolveBotLogin()
144-
if (botLogin) {
145-
logger.warn("GitHub App identity failed, using GH_TOKEN identity", { login: botLogin })
146-
Sentry.setTag("bot.login", botLogin)
147-
} else {
148-
logger.warn("could not resolve bot identity -- self-loop guard degraded")
149-
}
150-
}
151-
152-
// Re-normalize triggers with the resolved bot login for ignore_authors
153-
const normalizedTriggers = (cfg.triggers ?? []).map((t) => normalizeTrigger(t, botLogin))
154-
155152
const handlerContext: HandlerContext = {
156153
pipeline,
157154
dedup,
@@ -160,7 +157,7 @@ export async function bootstrap(opts: BootstrapOptions): Promise<BootstrapResult
160157
}
161158

162159
const app = createApp({
163-
handlers,
160+
handlers: normalizedHandlers,
164161
handlerContext,
165162
store,
166163
apiToken,
@@ -173,7 +170,7 @@ export async function bootstrap(opts: BootstrapOptions): Promise<BootstrapResult
173170
fetch: app.fetch,
174171
})
175172

176-
const triggerCount = normalizedTriggers.filter((t) => t.source === "github_app").length
173+
const triggerCount = triggers.filter((t) => t.source === "github_app").length
177174
logger.info("listening", {
178175
url: `http://0.0.0.0:${server.port}`,
179176
triggers: {
@@ -269,8 +266,8 @@ async function configureGitIdentity(botLogin: string): Promise<void> {
269266
await proc.exited
270267
}
271268

272-
await gitConfig([], "user.name", botLogin)
273-
await gitConfig([], "user.email", email)
269+
await gitConfig(["--global"], "user.name", botLogin)
270+
await gitConfig(["--global"], "user.email", email)
274271
await gitConfig(["-C", devDir], "user.name", botLogin)
275272
await gitConfig(["-C", devDir], "user.email", email)
276273

‎packages/opentower/src/entity.ts‎

Lines changed: 18 additions & 45 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,15 @@ export type EntityKey = {
1919
linkedIssues: number[]
2020
}
2121

22+
// PR-related events that all extract from payload.pull_request.
23+
const PR_EVENTS = new Set(["pull_request", "pull_request_review_comment", "pull_request_review"])
24+
25+
// CI events that extract from a pull_requests array inside the event object.
26+
const CI_EVENTS: Record<string, string> = {
27+
check_suite: "check_suite.pull_requests",
28+
workflow_run: "workflow_run.pull_requests",
29+
}
30+
2231
// Extract entity key from a GitHub webhook payload.
2332
// Returns null for events that don't map to a trackable entity.
2433
// When a GitHubFetcher is provided, events that lack full context
@@ -54,22 +63,9 @@ export async function extractEntityKey(
5463
return { key: `${repo}#${num}`, repo, number: num, kind: "issue", linkedIssues: [] }
5564
}
5665

57-
// pull_request.*
58-
if (event === "pull_request") {
59-
const num = lookup(payload, "pull_request.number")
60-
if (typeof num !== "number") return null
61-
const body = lookupString(payload, "pull_request.body") ?? ""
62-
return {
63-
key: `${repo}#${num}`,
64-
repo,
65-
number: num,
66-
kind: "pull_request",
67-
linkedIssues: extractLinkedIssues(body, repo),
68-
}
69-
}
70-
71-
// pull_request_review_comment.*
72-
if (event === "pull_request_review_comment") {
66+
// pull_request, pull_request_review_comment, pull_request_review
67+
// All extract from payload.pull_request with identical logic.
68+
if (PR_EVENTS.has(event)) {
7369
const num = lookup(payload, "pull_request.number")
7470
if (typeof num !== "number") return null
7571
const body = lookupString(payload, "pull_request.body") ?? ""
@@ -82,34 +78,11 @@ export async function extractEntityKey(
8278
}
8379
}
8480

85-
// pull_request_review.*
86-
if (event === "pull_request_review") {
87-
const num = lookup(payload, "pull_request.number")
88-
if (typeof num !== "number") return null
89-
const body = lookupString(payload, "pull_request.body") ?? ""
90-
return {
91-
key: `${repo}#${num}`,
92-
repo,
93-
number: num,
94-
kind: "pull_request",
95-
linkedIssues: extractLinkedIssues(body, repo),
96-
}
97-
}
98-
99-
// check_suite.* -- extract from pull_requests array, fetch PR body for links
100-
if (event === "check_suite") {
101-
const prs = lookup(payload, "check_suite.pull_requests")
102-
if (!Array.isArray(prs) || prs.length === 0) return null
103-
const first = prs[0] as Record<string, unknown>
104-
const num = first?.number
105-
if (typeof num !== "number") return null
106-
const linkedIssues = fetcher ? await fetchLinkedIssues(fetcher, repo, num) : []
107-
return { key: `${repo}#${num}`, repo, number: num, kind: "pull_request", linkedIssues }
108-
}
109-
110-
// workflow_run.* -- extract from pull_requests array, fetch PR body for links
111-
if (event === "workflow_run") {
112-
const prs = lookup(payload, "workflow_run.pull_requests")
81+
// check_suite, workflow_run — extract from pull_requests array,
82+
// fetch PR body for linked issue detection.
83+
const ciPath = CI_EVENTS[event]
84+
if (ciPath) {
85+
const prs = lookup(payload, ciPath)
11386
if (!Array.isArray(prs) || prs.length === 0) return null
11487
const first = prs[0] as Record<string, unknown>
11588
const num = first?.number
@@ -118,7 +91,7 @@ export async function extractEntityKey(
11891
return { key: `${repo}#${num}`, repo, number: num, kind: "pull_request", linkedIssues }
11992
}
12093

121-
// push -- resolve branch to PR, or parse commit messages for issue refs
94+
// push — resolve branch to PR, or parse commit messages for issue refs
12295
if (event === "push") {
12396
return resolvePushEntity(payload, repo, fetcher)
12497
}

‎packages/opentower/src/github-api.ts‎

Lines changed: 57 additions & 59 deletions
Original file line numberDiff line numberDiff line change
@@ -21,67 +21,65 @@ export function createGitHubFetcher(token: string): GitHubFetcher {
2121
"X-GitHub-Api-Version": "2022-11-28",
2222
}
2323

24-
return {
25-
async fetchPR(repo, number) {
26-
try {
27-
const res = await fetch(`${GITHUB_API}/repos/${repo}/pulls/${number}`, { headers })
28-
if (!res.ok) return null
29-
const data = (await res.json()) as { body: string | null }
30-
return { body: data.body }
31-
} catch (err) {
32-
Sentry.logger.warn("github_api.fetch_pr_failed", {
33-
repo,
34-
number,
35-
error: formatError(err),
36-
})
37-
return null
38-
}
39-
},
24+
// Shared fetch helper for PR and issue endpoints. Avoids duplicating
25+
// the try/catch + Sentry logging pattern across fetchPR and fetchIssue.
26+
async function fetchByType(
27+
type: "pulls" | "issues",
28+
repo: string,
29+
number: number,
30+
): Promise<{ body: string | null } | null> {
31+
try {
32+
const res = await fetch(`${GITHUB_API}/repos/${repo}/${type}/${number}`, { headers })
33+
if (!res.ok) return null
34+
const data = (await res.json()) as { body: string | null }
35+
return { body: data.body }
36+
} catch (err) {
37+
Sentry.logger.warn(`github_api.fetch_${type === "pulls" ? "pr" : "issue"}_failed`, {
38+
repo,
39+
number,
40+
error: formatError(err),
41+
})
42+
return null
43+
}
44+
}
4045

41-
async fetchIssue(repo, number) {
42-
try {
43-
const res = await fetch(`${GITHUB_API}/repos/${repo}/issues/${number}`, { headers })
44-
if (!res.ok) return null
45-
const data = (await res.json()) as { body: string | null }
46-
return { body: data.body }
47-
} catch (err) {
48-
Sentry.logger.warn("github_api.fetch_issue_failed", {
49-
repo,
50-
number,
51-
error: formatError(err),
52-
})
53-
return null
54-
}
55-
},
46+
async function fetchPR(repo: string, number: number) {
47+
return fetchByType("pulls", repo, number)
48+
}
5649

57-
async fetchEntity(repo, number) {
58-
// Try PR first — issues API also returns PRs but with less detail
59-
const pr = await this.fetchPR(repo, number)
60-
if (pr) return { kind: "pull_request" as const, body: pr.body }
61-
const issue = await this.fetchIssue(repo, number)
62-
if (issue) return { kind: "issue" as const, body: issue.body }
63-
return null
64-
},
50+
async function fetchIssue(repo: string, number: number) {
51+
return fetchByType("issues", repo, number)
52+
}
6553

66-
async findPRForBranch(repo, branch) {
67-
try {
68-
const [owner] = repo.split("/")
69-
const res = await fetch(
70-
`${GITHUB_API}/repos/${repo}/pulls?head=${encodeURIComponent(`${owner}:${branch}`)}&state=open&per_page=1`,
71-
{ headers },
72-
)
73-
if (!res.ok) return null
74-
const data = (await res.json()) as Array<{ number: number; body: string | null }>
75-
if (data.length === 0) return null
76-
return { number: data[0].number, body: data[0].body }
77-
} catch (err) {
78-
Sentry.logger.warn("github_api.find_pr_failed", {
79-
repo,
80-
branch,
81-
error: formatError(err),
82-
})
83-
return null
84-
}
85-
},
54+
async function fetchEntity(repo: string, number: number) {
55+
// Try PR first — issues API also returns PRs but with less detail
56+
const pr = await fetchPR(repo, number)
57+
if (pr) return { kind: "pull_request" as const, body: pr.body }
58+
const issue = await fetchIssue(repo, number)
59+
if (issue) return { kind: "issue" as const, body: issue.body }
60+
return null
8661
}
62+
63+
async function findPRForBranch(repo: string, branch: string) {
64+
try {
65+
const [owner] = repo.split("/")
66+
const res = await fetch(
67+
`${GITHUB_API}/repos/${repo}/pulls?head=${encodeURIComponent(`${owner}:${branch}`)}&state=open&per_page=1`,
68+
{ headers },
69+
)
70+
if (!res.ok) return null
71+
const data = (await res.json()) as Array<{ number: number; body: string | null }>
72+
if (data.length === 0) return null
73+
return { number: data[0].number, body: data[0].body }
74+
} catch (err) {
75+
Sentry.logger.warn("github_api.find_pr_failed", {
76+
repo,
77+
branch,
78+
error: formatError(err),
79+
})
80+
return null
81+
}
82+
}
83+
84+
return { fetchPR, fetchIssue, fetchEntity, findPRForBranch }
8785
}

‎packages/opentower/src/migrations.ts‎

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -315,8 +315,11 @@ export function repairSchema(db: Database): RepairResult {
315315
break
316316
}
317317
case "missing_column": {
318-
db.exec(`ALTER TABLE ${issue.table} ADD COLUMN ${issue.column} TEXT`)
319-
result.fixed.push(`added column ${issue.table}.${issue.column}`)
318+
const table = EXPECTED_TABLES.find((t) => t.name === issue.table)
319+
const col = table?.columns.find((c) => c.name === issue.column)
320+
const colType = col?.type ?? "TEXT"
321+
db.exec(`ALTER TABLE ${issue.table} ADD COLUMN ${issue.column} ${colType}`)
322+
result.fixed.push(`added column ${issue.table}.${issue.column} (${colType})`)
320323
break
321324
}
322325
case "missing_index": {

‎packages/opentower/src/pipeline.ts‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -349,10 +349,10 @@ export function makePipeline(opts: {
349349
// call — createAndPrompt will start its own.
350350
drainCounter.end()
351351

352-
// Retry with a brand-new session.
352+
// Retry with a brand-new session. Don't start an abort timer
353+
// here — createAndPrompt sets its own after semaphore acquisition
354+
// so wait time doesn't count against the timeout budget.
353355
const newAbort = new AbortController()
354-
const newAbortTimer = setTimeout(() => newAbort.abort(), timeoutMs)
355-
newAbortTimer.unref?.()
356356
const fresh: SessionEntry = {
357357
sessionId: "",
358358
entityKey: entityKey.key,
@@ -361,7 +361,7 @@ export function makePipeline(opts: {
361361
busy: true,
362362
queue: entry.queue,
363363
abort: newAbort,
364-
abortTimer: newAbortTimer,
364+
abortTimer: null as unknown as ReturnType<typeof setTimeout>,
365365
batchTimer: null,
366366
batchStart: null,
367367
idleTimer: null,

‎packages/opentower/src/schema.ts‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,9 @@ export const entities = sqliteTable(
88
entity_key: text("entity_key").primaryKey(),
99
repo: text("repo").notNull(),
1010
number: integer("number").notNull(),
11+
// The enum restricts values at the TypeScript level only. The SQL-level
12+
// CHECK(kind IN ('issue','pull_request')) constraint comes from the
13+
// migration SQL in migrations.ts, which is the actual DB-level guard.
1114
kind: text("kind", { enum: ["issue", "pull_request"] }).notNull(),
1215
session_id: text("session_id").notNull(),
1316
share_url: text("share_url"),

‎packages/opentower/src/storage.ts‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -170,6 +170,16 @@ export function openLifecycleStore(dbPath: string): LifecycleStore {
170170

171171
const db = drizzle(sqlite, { schema })
172172

173+
// Some methods use raw sqlite.prepare() instead of Drizzle:
174+
// - upsertEntity: COALESCE/CASE WHEN in ON CONFLICT not expressible in Drizzle
175+
// - addLink: INSERT OR IGNORE not expressible in Drizzle for composite PKs
176+
// - pruneOlderThan: bulk transactional deletes with subqueries, cleaner in raw SQL
177+
// - setRetentionDays: simple upsert, kept raw for consistency with getRetentionDays
178+
//
179+
// Timestamps: raw SQL uses datetime('now') (SQLite clock, UTC). Drizzle
180+
// .values() calls use the JS now() helper which produces the same
181+
// "YYYY-MM-DD HH:MM:SS" format in UTC. Both are equivalent when the
182+
// process runs with TZ=UTC (the default in the Docker container).
173183
return {
174184
upsertEntity(row) {
175185
sqlite

‎skills/auto-merge/SKILL.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -60,7 +60,7 @@ needs human review and list which criteria it exceeded.
6060
0. **Check quiet period**. Verify the PR was marked ready at least
6161
10 minutes ago with no reviewer activity since:
6262
```sh
63-
READY_AT=$(gh api "repos/<owner>/<repo>/issues/<N>/events" \
63+
READY_AT=$(gh api "repos/<owner>/<repo>/issues/<N>/timeline" --paginate \
6464
--jq '[.[] | select(.event=="ready_for_review")] | last | .created_at')
6565
```
6666
Calculate elapsed time. If less than 10 minutes have passed,

0 commit comments

Comments
 (0)