feat(gastown): configurable merge strategy (direct/PR)#701
feat(gastown): configurable merge strategy (direct/PR)#701
Conversation
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
Code Review SummaryStatus: 6 Open Issues Remaining | Recommendation: Address before merge OverviewThis PR adds a configurable merge strategy (
Remaining Open Issues (click to expand)WARNING
SUGGESTION
Fixed Issues (click to expand)Fixed in f114522
Fixed in 7245b3d
Fixed in 406b932
Fixed in 4c6f1e7
Fixed in 7eede6a
Files Reviewed (25 files)
|
jrf0110
force-pushed
the
473-configurable-merge-strategy
branch
5 times, most recently
from
3846180 to
b3274c2
Compare
| // host.docker.internal → host proxy mapping. Use the Docker | ||
| // Desktop VM host gateway IP directly so containers can reach | ||
| // the host's dev servers. | ||
| "KILO_API_URL": "http://192.168.65.254:3000", |
There was a problem hiding this comment.
[SUGGESTION]: Hardcoded 192.168.65.254 is macOS Docker Desktop-specific
This IP is the Docker Desktop VM host gateway on macOS. On Linux Docker (where host.docker.internal may not exist either), the host gateway is typically 172.17.0.1 or configured via --add-host. Consider adding a comment noting this is macOS-only, or using an environment variable / .dev.vars override so Linux developers don't have to modify wrangler.jsonc.
jrf0110
force-pushed
the
473-configurable-merge-strategy
branch
2 times, most recently
from
091068c to
b01e42a
Compare
| ), | ||
| ]; | ||
| if (rows.length === 0) return false; | ||
| const row = rows[0] as Record<string, unknown>; |
There was a problem hiding this comment.
[WARNING]: Uses TypeScript as cast, which the project coding style strongly discourages
The as Record<string, unknown> cast bypasses type safety. Since query() returns SqlStorageCursor, you could use a Zod schema to parse the row instead:
const ApprovedRow = z.object({ approved: z.union([z.boolean(), z.number()]).nullable() });
const row = ApprovedRow.parse(rows[0]);
return row.approved === true || row.approved === 1;Alternatively, if the overhead isn't justified here, at least narrow with a runtime check rather than as.
| const pkg = JSON.parse(fs.readFileSync("package.json", "utf8")); | ||
|
|
||
| // catalog: → concrete versions (keep in sync with root pnpm-workspace.yaml) | ||
| const catalog = { |
There was a problem hiding this comment.
[WARNING]: Hardcoded catalog versions must be manually kept in sync with pnpm-workspace.yaml
These version strings are duplicated from the root workspace catalog. If someone updates a catalog version in pnpm-workspace.yaml but forgets to update this file, the container will install a different version than the rest of the monorepo.
Consider either:
- Adding a CI check that compares these values against
pnpm-workspace.yaml - Generating this map from
pnpm-workspace.yamlat build time (e.g.yqor a small Node script that reads the YAML) - At minimum, adding a comment with the path to the source of truth:
pnpm-workspace.yaml→catalog:section
| # bun install. Versions are defined in resolve-catalog.sh and must be kept | ||
| # in sync with the root pnpm-workspace.yaml. | ||
| COPY package.json bun.lock resolve-catalog.sh ./ | ||
| RUN sh resolve-catalog.sh && bun install --production |
There was a problem hiding this comment.
[SUGGESTION]: --frozen-lockfile was removed — builds are no longer deterministic
The old bun install --frozen-lockfile --production ensured the lockfile matched package.json exactly. Now that resolve-catalog.sh rewrites package.json at build time, the lockfile won't match and --frozen-lockfile would fail.
This is an intentional tradeoff, but it means bun install can now resolve different versions across builds. Consider regenerating bun.lock after running resolve-catalog.sh and committing that, or documenting this tradeoff in the Dockerfile comment.
jrf0110
force-pushed
the
473-configurable-merge-strategy
branch
from
b01e42a to
8d28245
Compare
| return 'open'; | ||
| } | ||
|
|
||
| return null; |
There was a problem hiding this comment.
[WARNING]: checkPRStatus silently returns null for unrecognized PR URL formats — MR beads stuck forever
If the prUrl doesn't match the GitHub (/github.com/.../pull/\d+) or GitLab (/.../merge_requests/\d+) regex patterns — e.g. a GitHub Enterprise URL like https://github.mycompany.com/org/repo/pull/42 — this method returns null. The caller (pollPendingPRs) treats null as "skip", so the MR bead stays in in_progress indefinitely with no warning logged.
Consider logging a warning when neither regex matches, so operators can diagnose stuck MR beads:
console.warn(`${TOWN_LOG} checkPRStatus: unrecognized PR URL format: ${prUrl}`);
return null;| // Create a production-only package.json that strips workspace: references | ||
| // (they can't be resolved outside the monorepo). | ||
| const pkg = JSON.parse(readFileSync(resolve(containerDir, 'package.json'), 'utf8')); | ||
| if (pkg.devDependencies) { |
There was a problem hiding this comment.
[SUGGESTION]: Only strips workspace: references from devDependencies, not dependencies
If a production dependency in container/package.json ever uses a workspace: specifier (currently they use catalog: so this is fine today), the generated package.prod.json would still contain the unresolvable reference and pnpm install --prod would fail inside the container.
Consider also stripping workspace: from dependencies for robustness:
for (const section of ['dependencies', 'devDependencies']) {
if (pkg[section]) {
for (const [name, version] of Object.entries(pkg[section])) {
if (typeof version === 'string' && version.startsWith('workspace:')) {
delete pkg[section][name];
}
}
}
}
jrf0110
force-pushed
the
473-configurable-merge-strategy
branch
from
8d28245 to
69680fe
Compare
| ${beads.columns.updated_at} = ? | ||
| WHERE ${beads.bead_id} = ? | ||
| `, | ||
| [new Date().toISOString(), entryId] |
There was a problem hiding this comment.
[SUGGESTION]: Inconsistent timestamp helper — uses new Date().toISOString() instead of the now() helper
Every other function in this file uses the local now() helper (defined at line 27) for consistency. This one-off new Date().toISOString() call does the same thing but breaks the pattern, making it easy to miss if now() is ever changed (e.g. to use a monotonic clock or add logging).
| [new Date().toISOString(), entryId] | |
| [now(), entryId] |
| } | ||
| ); | ||
|
|
||
| if (!response.ok) { |
There was a problem hiding this comment.
[WARNING]: No special handling for HTTP 422 (duplicate PR) — MR bead marked as failed even though the PR exists
If a PR already exists for the same head→base pair, GitHub returns 422 with "A pull request already exists". This can happen if the alarm retries after a transient network failure where the PR was created but the response was lost.
The current code throws a generic error, which causes triggerPRCreation to mark the MR bead as failed — even though the PR actually exists on GitHub. The bead is then stuck in failed state with no pr_url, so pollPendingPRs never picks it up.
Consider detecting 422 and attempting to find the existing PR via the GitHub search/list API, or at minimum logging a distinct error so operators can manually recover.
| * .default() during parsing, injecting phantom values (e.g. merge_strategy: | ||
| * 'direct') that overwrite existing config on partial updates. | ||
| */ | ||
| export const TownConfigUpdateSchema = z.object({ |
There was a problem hiding this comment.
[WARNING]: TownConfigUpdateSchema is manually duplicated from TownConfigSchema — no compile-time drift detection
The comment at line 246 explains why .partial() can't be used (Zod fires .default() during parsing). However, this manual duplication means any new field added to TownConfigSchema must also be manually added here. There's no compile-time check to catch drift.
Consider adding a type-level assertion to catch missing fields, e.g.:
type _AssertUpdateCoversConfig = keyof TownConfig extends keyof TownConfigUpdate ? true : never;
const _: _AssertUpdateCoversConfig = true;This would cause a compile error if TownConfigSchema gains a field that TownConfigUpdateSchema doesn't have.
Add a merge_strategy config field (direct | pr) at the town and rig level. When set to 'pr', the Refinery creates a GitHub PR or GitLab MR instead of pushing directly to the default branch, enabling human review of agent work before it lands. Schema & Config: - MergeStrategy enum and merge_strategy field on TownConfigSchema - Per-rig override on RigConfig with town-level inheritance - Mirrored schema in Next.js gastown client PR/MR Creation (platform-pr.util.ts): - parseGitUrl with GitLab subgroup support and credential stripping - createGitHubPR / createGitLabMR via REST APIs - buildPRBody with quality gate results and agent attribution - GitHubPRStatusSchema / GitLabMRStatusSchema for polling Review Queue & Town DO: - setReviewPrUrl, markReviewInReview, listPendingPRReviews - executeMergeStrategy dispatches to direct merge or PR creation - triggerPRCreation with fail-fast on missing tokens (no silent fallback to direct merge) - pollPendingPRs with rate limiting (10 PRs/tick) - recoverStuckReviews excludes PR-strategy beads - Sanitize git URLs in logs and bead event metadata Refinery & Polecat Prompts: - Refinery prompt accepts mergeStrategy param - Polecat prompt explicitly prohibits PR/MR creation Dashboard UI: - Merge strategy radio picker in town settings - PR link on merge_request beads in BeadDetailDrawer Dev Infrastructure: - Fix container networking: use Docker Desktop host gateway IP - Bind wrangler dev to 0.0.0.0 - Resolve catalog: references in Dockerfiles for bun compatibility - Authenticate gh CLI via GH_TOKEN from git credentials - Fail fast when JWT minting fails - Return null for falsy JWT secrets 30 unit tests covering merge strategy resolution, git URL parsing (including subgroups, SSH, credential-bearing URLs), and PR body generation.
jrf0110
force-pushed
the
473-configurable-merge-strategy
branch
from
69680fe to
7eede6a
Compare
1 participant
Summary
Implements #473 — Configurable Merge Strategy, a sub-issue of #204.
merge_strategyconfig field (direct|pr) at the town and rig level, with rig-level overrides inheriting from the town defaultpr, the Refinery creates a GitHub PR or GitLab MR instead of pushing directly to the default branch, enabling human review before agent work landsChanges by area
Schema & Config
MergeStrategyZod enum andmerge_strategyfield onTownConfigSchema(defaults todirect)merge_strategyoverride onRigConfigresolveMergeStrategy()helper: rig override → town default →directPR/MR Creation (
platform-pr.util.ts— new)parseGitUrl()— extracts platform/owner/repo from HTTPS and SSH git URLscreateGitHubPR()/createGitLabMR()— REST API calls with labelsbuildPRBody()— Markdown body with quality gate results and agent attributionReview Queue & Town DO
setReviewPrUrl(),markReviewInReview(),listPendingPRReviews()on review queueexecuteMergeStrategy()dispatches to direct merge or PR creationtriggerPRCreation()creates the PR and tracks the URL on the MR beadpollPendingPRs()+checkPRStatus()poll GitHub/GitLab APIs for PR statepr_createdandpr_creation_failedbead event typesRefinery Prompt
mergeStrategyparam; whenpr, instructs the refinery to push the branch and callgt_done(TownDO creates the PR) instead of merging itselfDashboard UI
BeadDetailDrawerDev Infrastructure
192.168.65.254) instead ofhost.docker.internalwhich doesn't work on wrangler'sworkerd-network0.0.0.0so containers can reach the workercatalog:references in Dockerfiles for bun compatibilityresolveJWTSecretTests — 23 unit tests covering merge strategy resolution, git URL parsing, and PR body generation
How to test
merge_strategy: "pr"in town settingsCloses #473