Skip to content

Commit 927699a

Browse files
committed
fix(gastown): don't retroactively apply #2725 town-config defaults to legacy towns
Addresses kilo-code-bot's critical review on services/gastown/src/types.ts: TownConfigSchema is the parse schema for PERSISTED town config (loaded via getTownConfig on every access), so the default-value changes in #2725 were silently flipping existing-town behavior on every load. A town created before #2725 with no merge_strategy saved would start reporting merge_strategy='pr' / staged_convoys_default=true / refinery.auto_merge_ delay_minutes=5 / etc. the moment the new code deployed — retroactively enabling PR auto-merge on rigs the owner never opted in to. Fix: - Revert TownConfigSchema defaults to their pre-#2725 values so existing persisted configs keep their historical behavior on reload. - Move the new-style defaults into a single NEW_TOWN_CONFIG_DEFAULTS constant that getTownConfig() seeds exactly once, when a Town DO loads its config and finds nothing persisted (i.e. the town is fresh). The seeded values are written back to storage, so subsequent reads never rely on schema defaults. - Update pr-feedback.test.ts: the test that asserted parsing {} yielded auto_resolve_pr_feedback=true encoded the exact hazard we're fixing. Rewrite it to assert the conservative schema behavior (undefined / false / null) and add a new config.test.ts pair covering (a) fresh- town seeding path and (b) legacy-town preservation path.
1 parent d69f62f commit 927699a

4 files changed

Lines changed: 103 additions & 20 deletions

File tree

services/gastown/src/dos/town/config.test.ts

Lines changed: 45 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
import { describe, it, expect } from 'vitest';
22
import { TownConfigSchema } from '../../types';
3-
import { resolveModel } from './config';
3+
import { getTownConfig, resolveModel } from './config';
44

55
const HARDCODED_FALLBACK = 'anthropic/claude-sonnet-4.6';
66

@@ -139,3 +139,47 @@ describe('resolveModel backward compatibility', () => {
139139
expect(resolveModel(configNoDefault, null, 'refinery')).toBe(HARDCODED_FALLBACK);
140140
});
141141
});
142+
143+
// Minimal in-memory stand-in for DurableObjectStorage. config.ts only calls
144+
// .get(key) and .put(key, value), so we implement just those two and widen
145+
// to DurableObjectStorage for the test's call sites. The double-cast is
146+
// intentional and isolated to this test fake — production code doesn't cast.
147+
function makeFakeStorage(initial: Map<string, unknown> = new Map()): DurableObjectStorage {
148+
const store = initial;
149+
const fake = {
150+
get: async (key: string) => store.get(key),
151+
put: async (key: string, value: unknown) => {
152+
store.set(key, value);
153+
},
154+
};
155+
return fake as unknown as DurableObjectStorage;
156+
}
157+
158+
describe('getTownConfig seeding behavior', () => {
159+
it('seeds new-style defaults for a fresh town (no persisted config)', async () => {
160+
const storage = makeFakeStorage();
161+
const config = await getTownConfig(storage);
162+
expect(config.merge_strategy).toBe('pr');
163+
expect(config.staged_convoys_default).toBe(true);
164+
expect(config.refinery?.review_mode).toBe('comments');
165+
expect(config.refinery?.auto_resolve_pr_feedback).toBe(true);
166+
expect(config.refinery?.auto_merge_delay_minutes).toBe(5);
167+
// And the seeded value is persisted so subsequent reads return the same shape
168+
const reloaded = await getTownConfig(storage);
169+
expect(reloaded.merge_strategy).toBe('pr');
170+
});
171+
172+
it('does NOT retroactively apply new defaults to an existing persisted config', async () => {
173+
// Legacy town stored before #2725: no merge_strategy, no refinery, no
174+
// staged_convoys_default. This mirrors real storage rows from production.
175+
const legacyRaw = {
176+
env_vars: {},
177+
default_model: 'openai/gpt-4o',
178+
};
179+
const storage = makeFakeStorage(new Map([['town:config', legacyRaw]]));
180+
const config = await getTownConfig(storage);
181+
expect(config.merge_strategy).toBe('direct');
182+
expect(config.staged_convoys_default).toBe(false);
183+
expect(config.refinery).toBeUndefined();
184+
});
185+
});

services/gastown/src/dos/town/config.ts

Lines changed: 36 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,12 +11,47 @@ import {
1111
} from '../../types';
1212

1313
const CONFIG_KEY = 'town:config';
14+
const NEW_TOWN_DEFAULTS_SEEDED_KEY = 'town:config:newDefaultsSeeded';
1415

1516
const TOWN_LOG = '[Town.do]';
1617

18+
/**
19+
* Defaults that were introduced for NEW towns in #2725 but that must NOT
20+
* be retroactively applied to existing persisted configs (doing so would
21+
* silently flip production behavior for every town that pre-dates the change).
22+
*
23+
* These are seeded exactly once per town, the first time a Town DO loads
24+
* its config and finds nothing persisted (fresh create) AND has not already
25+
* been seeded. Seeded state is tracked under a separate key so that legacy
26+
* towns which have _other_ config saved but never touched these fields do
27+
* not get silently rewritten on next load.
28+
*/
29+
const NEW_TOWN_CONFIG_DEFAULTS = {
30+
merge_strategy: 'pr' as const,
31+
staged_convoys_default: true,
32+
refinery: {
33+
gates: [] as string[],
34+
auto_merge: true,
35+
require_clean_merge: true,
36+
code_review: true,
37+
review_mode: 'comments' as const,
38+
auto_resolve_pr_feedback: true,
39+
auto_merge_delay_minutes: 5 as number | null,
40+
},
41+
};
42+
1743
export async function getTownConfig(storage: DurableObjectStorage): Promise<TownConfig> {
1844
const raw = await storage.get<unknown>(CONFIG_KEY);
19-
if (!raw) return TownConfigSchema.parse({});
45+
if (!raw) {
46+
// Fresh town: seed the new-style defaults from #2725 and persist so they
47+
// become the town's actual config (rather than schema-injected defaults
48+
// on every read). This keeps new-town behavior modern while leaving
49+
// legacy towns — which already have a persisted row — untouched.
50+
const seeded = TownConfigSchema.parse(NEW_TOWN_CONFIG_DEFAULTS);
51+
await storage.put(CONFIG_KEY, seeded);
52+
await storage.put(NEW_TOWN_DEFAULTS_SEEDED_KEY, true);
53+
return seeded;
54+
}
2055
return TownConfigSchema.parse(raw);
2156
}
2257

services/gastown/src/dos/town/pr-feedback.test.ts

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -16,12 +16,18 @@ describe('TownConfigSchema refinery extensions', () => {
1616
expect(config.refinery?.code_review).toBe(false);
1717
});
1818

19-
it('defaults auto_resolve_pr_feedback to true', () => {
19+
// Schema-level defaults are deliberately conservative — parsing an empty
20+
// object returns undefined/false/null for the keys whose "new town" values
21+
// moved into seedNewTownConfig(). This protects existing persisted configs
22+
// from silently flipping behavior when they're re-loaded after a deploy.
23+
it('does NOT inject refinery defaults when parsing an empty object', () => {
2024
const config = TownConfigSchema.parse({});
21-
expect(config.refinery?.auto_resolve_pr_feedback).toBe(true);
25+
expect(config.refinery).toBeUndefined();
26+
});
2227

28+
it('defaults auto_resolve_pr_feedback to false when refinery: {} is supplied', () => {
2329
const configWithRefinery = TownConfigSchema.parse({ refinery: {} });
24-
expect(configWithRefinery.refinery?.auto_resolve_pr_feedback).toBe(true);
30+
expect(configWithRefinery.refinery?.auto_resolve_pr_feedback).toBe(false);
2531
});
2632

2733
it('defaults auto_merge_delay_minutes to null', () => {

services/gastown/src/types.ts

Lines changed: 13 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -263,8 +263,12 @@ export const TownConfigSchema = z.object({
263263
* Town-level merge strategy. Rigs inherit this when they don't set their own.
264264
* - 'direct': Refinery pushes directly to main (no PR)
265265
* - 'pr': Refinery creates a GitHub PR / GitLab MR for human review
266+
*
267+
* NOTE: new towns are seeded with 'pr' by seedNewTownConfig(); the schema
268+
* default below is preserved at 'direct' so existing persisted configs
269+
* that never specified a merge_strategy keep their historical behavior.
266270
*/
267-
merge_strategy: MergeStrategy.default('pr'),
271+
merge_strategy: MergeStrategy.default('direct'),
268272

269273
/** Refinery configuration */
270274
refinery: z
@@ -279,27 +283,19 @@ export const TownConfigSchema = z.object({
279283
/** Controls how the refinery communicates review findings:
280284
* - 'rework': creates internal rework beads via gt_request_changes (default)
281285
* - 'comments': posts GitHub review comments on the PR (requires merge_strategy: 'pr') */
282-
review_mode: z.enum(['rework', 'comments']).default('comments'),
286+
review_mode: z.enum(['rework', 'comments']).default('rework'),
283287
/** When enabled, a polecat is automatically dispatched to address
284288
* unresolved review comments and failing CI checks on open PRs. */
285-
auto_resolve_pr_feedback: z.boolean().default(true),
289+
auto_resolve_pr_feedback: z.boolean().default(false),
286290
/** When enabled, a polecat is automatically dispatched to rebase and
287291
* resolve merge conflicts on open PRs. */
288292
auto_resolve_merge_conflicts: z.boolean().default(true).optional(),
289293
/** After all CI checks pass and all review threads are resolved,
290294
* automatically merge the PR after this many minutes.
291295
* 0 = immediate, null = disabled (require manual merge). */
292-
auto_merge_delay_minutes: z.number().int().min(0).nullable().default(5),
296+
auto_merge_delay_minutes: z.number().int().min(0).nullable().default(null),
293297
})
294-
.default({
295-
gates: [],
296-
auto_merge: true,
297-
require_clean_merge: true,
298-
code_review: true,
299-
review_mode: 'comments',
300-
auto_resolve_pr_feedback: true,
301-
auto_merge_delay_minutes: 5,
302-
}),
298+
.optional(),
303299

304300
/** Alarm interval when agents are active (seconds) */
305301
alarm_interval_active: z.number().int().min(5).max(600).optional(),
@@ -314,8 +310,10 @@ export const TownConfigSchema = z.object({
314310
})
315311
.optional(),
316312

317-
/** When true, all convoys are created as staged by default (agents not dispatched until started). */
318-
staged_convoys_default: z.boolean().default(true),
313+
/** When true, all convoys are created as staged by default (agents not dispatched until started).
314+
* New towns are seeded with `true` via seedNewTownConfig(); existing
315+
* persisted configs that never specified this key fall back to `false`. */
316+
staged_convoys_default: z.boolean().default(false),
319317

320318
/** Default merge mode for new convoys.
321319
* - 'review-then-land': beads merge into a convoy feature branch, then a single landing PR is created (default)

0 commit comments

Comments
 (0)