From dac091c022679076f5e74ef029c0e7d6b1908a63 Mon Sep 17 00:00:00 2001 From: Clayton Date: Tue, 7 Jul 2026 04:30:55 -0500 Subject: [PATCH] feat(config): add review.shared_config operator overlay (#2046) Wire parsed review overlay with sharedConfigSource provenance for the container-private _shared base manifest, plus loader warnings and docs. Co-authored-by: Cursor --- .gittensory.yml.example | 7 + config/examples/README.md | 4 +- .../gittensory-engine/src/focus-manifest.ts | 160 +++++++++++++++++- src/selfhost/private-config.ts | 119 +++++++++---- src/signals/focus-manifest-loader.ts | 26 ++- src/signals/focus-manifest.ts | 2 + test/unit/focus-manifest-loader.test.ts | 27 +++ test/unit/focus-manifest.test.ts | 46 ++++- test/unit/private-config.test.ts | 140 +++++++++++---- test/unit/selfhost-config-examples.test.ts | 8 +- test/unit/signals-coverage.test.ts | 2 +- 11 files changed, 469 insertions(+), 72 deletions(-) diff --git a/.gittensory.yml.example b/.gittensory.yml.example index ff3eb27a64..9aa5b9e435 100644 --- a/.gittensory.yml.example +++ b/.gittensory.yml.example @@ -342,6 +342,13 @@ gate: # Review output controls. These tune review output without changing the # deterministic gate policy above. Omit the block to keep the byte-identical # defaults. +# +# SELF-HOST ONLY (`review.shared_config`, #2046): when `GITTENSORY_REPO_CONFIG_DIR` is mounted, +# place a shared review base at `${GITTENSORY_REPO_CONFIG_DIR}/_shared/.gittensory.yml` (see +# `config/examples/shared.gittensory.yml`). Per-repo `review:` keys overlay it field-by-field — +# repo value wins when set, shared fills gaps, defaults stay byte-identical. Absent shared base is +# the common case and changes nothing. A malformed shared base warns and is ignored (never blocks a +# review). The loader records provenance at runtime in `review.sharedConfigSource` (not a YAML key). review: # Deterministic AI review eligibility filters (`review.auto_review`, #1954 / #2038–#2065). Each knob quietly # skips the advisory AI review for matching PRs — never a gate failure. When the Orb review check is enabled, diff --git a/config/examples/README.md b/config/examples/README.md index 76ffeaaeab..4f364e81b9 100644 --- a/config/examples/README.md +++ b/config/examples/README.md @@ -162,7 +162,9 @@ folded across one more layer; it is not a new merge algorithm. behavior is byte-identical to the pre-#1959 2-layer chain. A malformed or unreadable shared file fails safe exactly like a malformed per-repo or global file always has: it is dropped from the merge and the remaining, still-valid layers combine as if it were never mounted — a broken shared -base never blocks a review. +base never blocks a review. When a shared `review:` block contributes, the parsed manifest carries +`review.sharedConfigSource` (runtime provenance only, #2046) with the relative path of the shared +file that supplied the base layer. ### Example 4 — shared base + global default + a per-repo override, all three present diff --git a/packages/gittensory-engine/src/focus-manifest.ts b/packages/gittensory-engine/src/focus-manifest.ts index 1ef18cdb85..5e58199b49 100644 --- a/packages/gittensory-engine/src/focus-manifest.ts +++ b/packages/gittensory-engine/src/focus-manifest.ts @@ -527,6 +527,10 @@ export type FocusManifestReviewConfig = { * ONLY (#2173, for #1961): parsed + normalized here; the merge/close decision that reads this mode is a separate * maintainer-only slice. null (default, absent) ⇒ byte-identical to today. */ linkedIssueSatisfaction: LinkedIssueSatisfactionMode | null; + /** Runtime provenance when the container-private shared base (`review.shared_config`, #2046) filled review + * fields from `GITTENSORY_REPO_CONFIG_DIR/_shared/.gittensory.yml`. Never parsed from maintainer YAML — + * set by the private-config loader only. null (default) ⇒ no shared overlay was applied. */ + sharedConfigSource: string | null; }; /** `review.linkedIssueSatisfaction` modes (#2173). `off` = not evaluated (same as unset). */ @@ -847,7 +851,7 @@ const EMPTY_MANIFEST: FocusManifest = { publicNotes: [], gate: { ...EMPTY_GATE_CONFIG }, settings: {}, - review: { present: false, footerText: null, note: null, fields: {}, enrichmentAnalyzers: {}, profile: null, tone: null, securityFocus: null, inlineComments: null, fixHandoff: null, autoMergeSummary: null, suggestions: null, changedFilesSummary: null, effortScore: null, testGeneration: null, impactMap: null, cultureProfile: null, reviewMemory: null, findingCategories: null, inlineCommentsPerCategory: null, minFindingSeverity: null, maxFindings: { ...EMPTY_MAX_FINDINGS_CONFIG }, commentVerbosity: null, pathInstructions: [], instructions: null, excludePaths: [], pathFilters: [], preMergeChecks: [], autoReview: { ...EMPTY_AUTO_REVIEW_CONFIG }, labelingRules: [], aiModel: { ...EMPTY_SELF_HOST_AI_MODEL_CONFIG }, visual: { ...EMPTY_VISUAL_CONFIG }, linkedIssueSatisfaction: null }, + review: { present: false, footerText: null, note: null, fields: {}, enrichmentAnalyzers: {}, profile: null, tone: null, securityFocus: null, inlineComments: null, fixHandoff: null, autoMergeSummary: null, suggestions: null, changedFilesSummary: null, effortScore: null, testGeneration: null, impactMap: null, cultureProfile: null, reviewMemory: null, findingCategories: null, inlineCommentsPerCategory: null, minFindingSeverity: null, maxFindings: { ...EMPTY_MAX_FINDINGS_CONFIG }, commentVerbosity: null, pathInstructions: [], instructions: null, excludePaths: [], pathFilters: [], preMergeChecks: [], autoReview: { ...EMPTY_AUTO_REVIEW_CONFIG }, labelingRules: [], aiModel: { ...EMPTY_SELF_HOST_AI_MODEL_CONFIG }, visual: { ...EMPTY_VISUAL_CONFIG }, linkedIssueSatisfaction: null, sharedConfigSource: null }, features: { ...EMPTY_FEATURES_CONFIG }, contentLane: { ...EMPTY_CONTENT_LANE_CONFIG }, repoDocGeneration: { ...EMPTY_REPO_DOC_GENERATION_CONFIG }, @@ -877,7 +881,7 @@ function emptyManifest(source: FocusManifestSource, warnings: string[] = []): Fo warnings, gate: { ...EMPTY_GATE_CONFIG }, settings: {}, - review: { present: false, footerText: null, note: null, fields: {}, enrichmentAnalyzers: {}, profile: null, tone: null, securityFocus: null, inlineComments: null, fixHandoff: null, autoMergeSummary: null, suggestions: null, changedFilesSummary: null, effortScore: null, testGeneration: null, impactMap: null, cultureProfile: null, reviewMemory: null, findingCategories: null, inlineCommentsPerCategory: null, minFindingSeverity: null, maxFindings: { ...EMPTY_MAX_FINDINGS_CONFIG }, commentVerbosity: null, pathInstructions: [], instructions: null, excludePaths: [], pathFilters: [], preMergeChecks: [], autoReview: { ...EMPTY_AUTO_REVIEW_CONFIG }, labelingRules: [], aiModel: { ...EMPTY_SELF_HOST_AI_MODEL_CONFIG }, visual: { ...EMPTY_VISUAL_CONFIG }, linkedIssueSatisfaction: null }, + review: { present: false, footerText: null, note: null, fields: {}, enrichmentAnalyzers: {}, profile: null, tone: null, securityFocus: null, inlineComments: null, fixHandoff: null, autoMergeSummary: null, suggestions: null, changedFilesSummary: null, effortScore: null, testGeneration: null, impactMap: null, cultureProfile: null, reviewMemory: null, findingCategories: null, inlineCommentsPerCategory: null, minFindingSeverity: null, maxFindings: { ...EMPTY_MAX_FINDINGS_CONFIG }, commentVerbosity: null, pathInstructions: [], instructions: null, excludePaths: [], pathFilters: [], preMergeChecks: [], autoReview: { ...EMPTY_AUTO_REVIEW_CONFIG }, labelingRules: [], aiModel: { ...EMPTY_SELF_HOST_AI_MODEL_CONFIG }, visual: { ...EMPTY_VISUAL_CONFIG }, linkedIssueSatisfaction: null, sharedConfigSource: null }, features: { ...EMPTY_FEATURES_CONFIG }, contentLane: { ...EMPTY_CONTENT_LANE_CONFIG }, repoDocGeneration: { ...EMPTY_REPO_DOC_GENERATION_CONFIG }, @@ -1878,7 +1882,7 @@ function parsePublicSafeText(value: JsonValue | undefined, field: string, warnin * throws; invalid/unsafe values are dropped with warnings. */ function parseReviewConfig(value: JsonValue | undefined, warnings: string[]): FocusManifestReviewConfig { - const empty: FocusManifestReviewConfig = { present: false, footerText: null, note: null, fields: {}, enrichmentAnalyzers: {}, profile: null, tone: null, securityFocus: null, inlineComments: null, fixHandoff: null, autoMergeSummary: null, suggestions: null, changedFilesSummary: null, effortScore: null, testGeneration: null, impactMap: null, cultureProfile: null, reviewMemory: null, findingCategories: null, inlineCommentsPerCategory: null, minFindingSeverity: null, maxFindings: { ...EMPTY_MAX_FINDINGS_CONFIG }, commentVerbosity: null, pathInstructions: [], instructions: null, excludePaths: [], pathFilters: [], preMergeChecks: [], autoReview: { ...EMPTY_AUTO_REVIEW_CONFIG }, labelingRules: [], aiModel: { ...EMPTY_SELF_HOST_AI_MODEL_CONFIG }, visual: { ...EMPTY_VISUAL_CONFIG }, linkedIssueSatisfaction: null }; + const empty: FocusManifestReviewConfig = { present: false, footerText: null, note: null, fields: {}, enrichmentAnalyzers: {}, profile: null, tone: null, securityFocus: null, inlineComments: null, fixHandoff: null, autoMergeSummary: null, suggestions: null, changedFilesSummary: null, effortScore: null, testGeneration: null, impactMap: null, cultureProfile: null, reviewMemory: null, findingCategories: null, inlineCommentsPerCategory: null, minFindingSeverity: null, maxFindings: { ...EMPTY_MAX_FINDINGS_CONFIG }, commentVerbosity: null, pathInstructions: [], instructions: null, excludePaths: [], pathFilters: [], preMergeChecks: [], autoReview: { ...EMPTY_AUTO_REVIEW_CONFIG }, labelingRules: [], aiModel: { ...EMPTY_SELF_HOST_AI_MODEL_CONFIG }, visual: { ...EMPTY_VISUAL_CONFIG }, linkedIssueSatisfaction: null, sharedConfigSource: null }; if (value === undefined || value === null) return empty; if (typeof value !== "object" || Array.isArray(value)) { warnings.push(`Manifest field "review" must be a mapping; ignoring it.`); @@ -2014,9 +2018,159 @@ function parseReviewConfig(value: JsonValue | undefined, warnings: string[]): Fo pathFilters, preMergeChecks, labelingRules, + sharedConfigSource: null, }; } +function pickOverlayNullable(override: T | null, base: T | null): T | null { + return override !== null ? override : base; +} + +function pickOverlayStringList(override: readonly string[], base: readonly string[]): string[] { + return override.length > 0 ? [...override] : [...base]; +} + +function pickOverlayPartialRecord( + override: Partial>, + base: Partial>, +): Partial> { + return { ...base, ...override }; +} + +function overlayMaxFindingsConfig(base: MaxFindingsConfig, override: MaxFindingsConfig): MaxFindingsConfig { + return { + blockers: pickOverlayNullable(override.blockers, base.blockers), + nits: pickOverlayNullable(override.nits, base.nits), + }; +} + +function overlayAutoReviewConfig(base: AutoReviewConfig, override: AutoReviewConfig): AutoReviewConfig { + return { + skipDrafts: pickOverlayNullable(override.skipDrafts, base.skipDrafts), + ignoreAuthors: pickOverlayStringList(override.ignoreAuthors, base.ignoreAuthors), + ignoreTitleKeywords: pickOverlayStringList(override.ignoreTitleKeywords, base.ignoreTitleKeywords), + skipLabels: pickOverlayStringList(override.skipLabels, base.skipLabels), + skipDocsOnly: pickOverlayNullable(override.skipDocsOnly, base.skipDocsOnly), + maxAddedLines: override.maxAddedLines > 0 ? override.maxAddedLines : base.maxAddedLines, + maxFiles: override.maxFiles > 0 ? override.maxFiles : base.maxFiles, + baseBranches: pickOverlayStringList(override.baseBranches, base.baseBranches), + autoPauseAfterReviewedCommits: pickOverlayNullable(override.autoPauseAfterReviewedCommits, base.autoPauseAfterReviewedCommits), + }; +} + +function overlaySelfHostAiModelConfig(base: SelfHostAiModelConfig, override: SelfHostAiModelConfig): SelfHostAiModelConfig { + return { + claudeModel: pickOverlayNullable(override.claudeModel, base.claudeModel), + claudeEffort: pickOverlayNullable(override.claudeEffort, base.claudeEffort), + codexModel: pickOverlayNullable(override.codexModel, base.codexModel), + codexEffort: pickOverlayNullable(override.codexEffort, base.codexEffort), + ollamaModel: pickOverlayNullable(override.ollamaModel, base.ollamaModel), + openaiModel: pickOverlayNullable(override.openaiModel, base.openaiModel), + openaiCompatibleModel: pickOverlayNullable(override.openaiCompatibleModel, base.openaiCompatibleModel), + anthropicModel: pickOverlayNullable(override.anthropicModel, base.anthropicModel), + }; +} + +function overlayVisualConfig(base: VisualConfig, override: VisualConfig): VisualConfig { + return { + preview: { urlTemplate: pickOverlayNullable(override.preview.urlTemplate, base.preview.urlTemplate) }, + routes: { + paths: pickOverlayStringList(override.routes.paths, base.routes.paths), + maxRoutes: pickOverlayNullable(override.routes.maxRoutes, base.routes.maxRoutes), + }, + themes: override.themes.length > 0 ? [...override.themes] : [...base.themes], + gif: override.gif ? override.gif : base.gif, + }; +} + +function computeReviewConfigPresent(review: Omit): boolean { + return ( + review.footerText !== null || + review.note !== null || + review.profile !== null || + review.tone !== null || + review.securityFocus !== null || + review.inlineComments !== null || + review.fixHandoff !== null || + review.autoMergeSummary !== null || + review.suggestions !== null || + review.changedFilesSummary !== null || + review.effortScore !== null || + review.testGeneration !== null || + review.impactMap !== null || + review.cultureProfile !== null || + review.reviewMemory !== null || + review.findingCategories !== null || + review.inlineCommentsPerCategory !== null || + review.minFindingSeverity !== null || + maxFindingsPresent(review.maxFindings) || + review.commentVerbosity !== null || + review.pathInstructions.length > 0 || + review.instructions !== null || + review.excludePaths.length > 0 || + review.pathFilters.length > 0 || + review.preMergeChecks.length > 0 || + autoReviewPresent(review.autoReview) || + review.labelingRules.length > 0 || + selfHostAiModelPresent(review.aiModel) || + visualConfigPresent(review.visual) || + review.linkedIssueSatisfaction !== null || + Object.keys(review.fields).length > 0 || + Object.keys(review.enrichmentAnalyzers).length > 0 + ); +} + +/** Overlay a higher-priority `review:` config onto a shared/base layer (#2046). Per-field: override wins when set; + * base fills gaps; defaults stay byte-identical. `sharedConfigSource` on the override is preserved when present. */ +export function overlayReviewConfig( + base: FocusManifestReviewConfig, + override: FocusManifestReviewConfig, +): FocusManifestReviewConfig { + const merged: FocusManifestReviewConfig = { + footerText: pickOverlayNullable(override.footerText, base.footerText), + note: pickOverlayNullable(override.note, base.note), + fields: pickOverlayPartialRecord(override.fields, base.fields), + enrichmentAnalyzers: pickOverlayPartialRecord(override.enrichmentAnalyzers, base.enrichmentAnalyzers), + profile: pickOverlayNullable(override.profile, base.profile), + tone: pickOverlayNullable(override.tone, base.tone), + securityFocus: pickOverlayNullable(override.securityFocus, base.securityFocus), + inlineComments: pickOverlayNullable(override.inlineComments, base.inlineComments), + fixHandoff: pickOverlayNullable(override.fixHandoff, base.fixHandoff), + autoMergeSummary: pickOverlayNullable(override.autoMergeSummary, base.autoMergeSummary), + suggestions: pickOverlayNullable(override.suggestions, base.suggestions), + changedFilesSummary: pickOverlayNullable(override.changedFilesSummary, base.changedFilesSummary), + effortScore: pickOverlayNullable(override.effortScore, base.effortScore), + testGeneration: pickOverlayNullable(override.testGeneration, base.testGeneration), + impactMap: pickOverlayNullable(override.impactMap, base.impactMap), + cultureProfile: pickOverlayNullable(override.cultureProfile, base.cultureProfile), + reviewMemory: pickOverlayNullable(override.reviewMemory, base.reviewMemory), + findingCategories: pickOverlayNullable(override.findingCategories, base.findingCategories), + inlineCommentsPerCategory: pickOverlayNullable(override.inlineCommentsPerCategory, base.inlineCommentsPerCategory), + minFindingSeverity: pickOverlayNullable(override.minFindingSeverity, base.minFindingSeverity), + maxFindings: overlayMaxFindingsConfig(base.maxFindings, override.maxFindings), + commentVerbosity: pickOverlayNullable(override.commentVerbosity, base.commentVerbosity), + pathInstructions: override.pathInstructions.length > 0 ? [...override.pathInstructions] : [...base.pathInstructions], + instructions: pickOverlayNullable(override.instructions, base.instructions), + excludePaths: pickOverlayStringList(override.excludePaths, base.excludePaths), + pathFilters: pickOverlayStringList(override.pathFilters, base.pathFilters), + preMergeChecks: override.preMergeChecks.length > 0 ? [...override.preMergeChecks] : [...base.preMergeChecks], + autoReview: overlayAutoReviewConfig(base.autoReview, override.autoReview), + labelingRules: override.labelingRules.length > 0 ? [...override.labelingRules] : [...base.labelingRules], + aiModel: overlaySelfHostAiModelConfig(base.aiModel, override.aiModel), + visual: overlayVisualConfig(base.visual, override.visual), + linkedIssueSatisfaction: pickOverlayNullable(override.linkedIssueSatisfaction, base.linkedIssueSatisfaction), + sharedConfigSource: override.sharedConfigSource ?? base.sharedConfigSource, + present: false, + }; + merged.present = computeReviewConfigPresent(merged); + return merged; +} + +/** Parse a raw `review:` mapping value. Exported for the private-config shared overlay (#2046). */ +export function parseReviewConfigMapping(value: JsonValue | undefined, warnings: string[]): FocusManifestReviewConfig { + return parseReviewConfig(value, warnings); +} + function maxFindingsPresent(config: MaxFindingsConfig): boolean { return config.blockers !== null || config.nits !== null; } diff --git a/src/selfhost/private-config.ts b/src/selfhost/private-config.ts index ca3ebd4d7f..dfb6779f1d 100644 --- a/src/selfhost/private-config.ts +++ b/src/selfhost/private-config.ts @@ -90,9 +90,18 @@ export function localConfigCandidates(repoFullName: string): string[] { /** Read the first candidate that exists, trying each in order; null when none do. A read error (ENOENT or * otherwise unreadable) is swallowed so the next candidate is tried. */ async function readFirstExisting(base: string, candidates: string[]): Promise { + const hit = await readFirstExistingWithPath(base, candidates); + return hit?.text ?? null; +} + +/** Like {@link readFirstExisting}, but also returns the winning relative candidate path (for provenance). */ +async function readFirstExistingWithPath( + base: string, + candidates: string[], +): Promise<{ text: string; path: string } | null> { for (const candidate of candidates) { try { - return await readFile(resolve(base, candidate), "utf8"); + return { text: await readFile(resolve(base, candidate), "utf8"), path: candidate }; } catch { // ENOENT / unreadable → try the next candidate } @@ -100,6 +109,30 @@ async function readFirstExisting(base: string, candidates: string[]): Promise): Record { + const { review: _review, ...rest } = mapping; + return rest; +} + +function extractReviewMapping(mapping: Record): Record | null { + const { review } = mapping; + if (review === undefined || review === null) return null; + if (typeof review === "object" && !Array.isArray(review)) return review as Record; + return null; +} + /** Tolerantly parse raw config text into a plain mapping for MERGE PURPOSES ONLY — same 2-line YAML/JSON detection * `parseFocusManifestContent` (focus-manifest.ts) uses, duplicated locally rather than exported from there so that * file's public surface stays unchanged for what is otherwise two lines of logic. Returns null — "not mergeable" — @@ -137,33 +170,51 @@ export function mergeConfigOverlay(base: unknown, override: unknown): unknown { return merged; } -/** Combine any number of config-text layers, given in ASCENDING priority order (lowest first, e.g. - * `[sharedText, globalText, repoText]` — #1959), into the raw text `parseFocusManifestContent` should parse. - * `null` entries (a layer whose file simply doesn't exist) are ignored outright. Every remaining, present layer is - * tolerantly parsed via {@link parseConfigMapping}; layers that parse as a mapping are folded left-to-right with - * {@link mergeConfigOverlay} (each later — higher-priority — layer overlays every earlier one), so a 3-way fold - * reduces to exactly the existing 2-way "per-repo overlays global" semantics when only 2 layers are present, and to - * a single file's raw text when only 1 is. A present-but-unparseable layer (malformed, oversized, or a parsed - * non-mapping value) is DROPPED from the fold — never blocks, never discards a still-valid sibling's policy — - * which is why fewer than 2 layers may end up parsed even when more than 2 are present on disk. With 0 parsed - * layers, the highest-priority PRESENT layer's raw text is returned unchanged (matching the original - * single-candidate priority, so a fully-broken set degrades exactly like a single malformed manifest always has). - * With exactly 1 parsed layer, that layer's own raw text is returned unchanged — never re-serialized — so a lone - * valid file's formatting/comments survive untouched, identical to the pre-#1959 "only one side present" case. - * Only 2+ successfully parsed layers are actually re-serialized as merged JSON. */ -function combineConfigLayers(layersAscendingPriority: (string | null)[]): string | null { - const present = layersAscendingPriority.filter((text): text is string => text !== null); - if (present.length === 0) return null; - const parsedLayers: { text: string; mapping: Record }[] = []; - for (const text of present) { - const mapping = parseConfigMapping(text); - if (mapping) parsedLayers.push({ text, mapping }); +/** Combine private-config layers with `review.shared_config` provenance (#2046). Non-`review` keys still deep-merge + * via {@link mergeConfigOverlay}; the `review` block is folded separately so provenance + warnings stay accurate. */ +function combineConfigLayersWithMeta( + layersAscendingPriority: Array<{ text: string | null; kind: ConfigLayerKind; sourcePath?: string | null }>, +): LocalManifestLoadResult { + const warnings: string[] = []; + let sharedConfigSource: string | null = null; + const present = layersAscendingPriority.filter((layer): layer is { text: string; kind: ConfigLayerKind; sourcePath?: string | null } => layer.text !== null); + if (present.length === 0) return { content: null, sharedConfigSource: null, warnings }; + + const sharedLayer = present.find((layer) => layer.kind === "shared"); + if (sharedLayer && parseConfigMapping(sharedLayer.text) === null) warnings.push(SHARED_BASE_MALFORMED_WARNING); + + const parsedLayers: Array<{ text: string; kind: ConfigLayerKind; mapping: Record; sourcePath: string | null }> = []; + for (const layer of present) { + const mapping = parseConfigMapping(layer.text); + if (mapping) parsedLayers.push({ text: layer.text, kind: layer.kind, mapping, sourcePath: layer.sourcePath ?? null }); + } + + if (parsedLayers.length === 0) { + return { content: present[present.length - 1]!.text, sharedConfigSource: null, warnings }; } - if (parsedLayers.length === 0) return present[present.length - 1]!; // none parsed → highest-priority present layer, raw - if (parsedLayers.length === 1) return parsedLayers[0]!.text; // exactly one parsed → its raw text, unchanged - let merged: unknown = parsedLayers[0]!.mapping; - for (const layer of parsedLayers.slice(1)) merged = mergeConfigOverlay(merged, layer.mapping); - return JSON.stringify(merged); + if (parsedLayers.length === 1) { + const only = parsedLayers[0]!; + if (only.kind === "shared" && extractReviewMapping(only.mapping) && only.sourcePath) { + sharedConfigSource = only.sourcePath; + } + return { content: only.text, sharedConfigSource, warnings }; + } + + let mergedBody: Record = stripReviewKey(parsedLayers[0]!.mapping); + for (const layer of parsedLayers.slice(1)) { + mergedBody = mergeConfigOverlay(mergedBody, stripReviewKey(layer.mapping)) as Record; + } + + let mergedReview: unknown; + for (const layer of parsedLayers) { + const review = extractReviewMapping(layer.mapping); + if (review === null) continue; + mergedReview = mergedReview === undefined ? review : mergeConfigOverlay(mergedReview, review); + if (layer.kind === "shared" && layer.sourcePath) sharedConfigSource = layer.sourcePath; + } + if (mergedReview !== undefined) mergedBody.review = mergedReview; + + return { content: JSON.stringify(mergedBody), sharedConfigSource, warnings }; } /** Build the container-local manifest reader over GITTENSORY_REPO_CONFIG_DIR, or null when the dir is unset/blank @@ -178,15 +229,21 @@ export function makeLocalManifestReader(dir: string | undefined): RepoFocusManif const trimmed = (dir ?? "").trim(); if (!trimmed) return null; const base = resolve(trimmed); - return async (repoFullName: string): Promise => { + return async (repoFullName: string): Promise => { const perRepo = localConfigCandidates(repoFullName); if (perRepo.length === 0) return null; // invalid repo name → no per-repo file, global default, or shared base - const [sharedText, globalText, repoText] = await Promise.all([ - readFirstExisting(base, SHARED_BASE_CONFIG_CANDIDATES), + const [sharedHit, globalText, repoText] = await Promise.all([ + readFirstExistingWithPath(base, SHARED_BASE_CONFIG_CANDIDATES), readFirstExisting(base, GLOBAL_CONFIG_CANDIDATES), readFirstExisting(base, perRepo), ]); - return combineConfigLayers([sharedText, globalText, repoText]); + const loaded = combineConfigLayersWithMeta([ + { text: sharedHit?.text ?? null, kind: "shared", sourcePath: sharedHit?.path ?? null }, + { text: globalText, kind: "global" }, + { text: repoText, kind: "repo" }, + ]); + if (loaded.content === null && loaded.warnings.length === 0 && loaded.sharedConfigSource === null) return null; + return loaded; }; } diff --git a/src/signals/focus-manifest-loader.ts b/src/signals/focus-manifest-loader.ts index a1e1240000..38388a8923 100644 --- a/src/signals/focus-manifest-loader.ts +++ b/src/signals/focus-manifest-loader.ts @@ -3,6 +3,7 @@ import type { JsonValue } from "../types"; import { nowIso } from "../utils/json"; import { contentLaneConfigToJson, featuresConfigToJson, gateConfigToJson, MAX_FOCUS_MANIFEST_BYTES, parseFocusManifest, parseFocusManifestContent, repoDocGenerationConfigToJson, reviewConfigToJson, reviewRecapConfigToJson, settingsOverrideToJson, type FocusManifest, type FocusManifestSource, type RepoReviewContext } from "./focus-manifest"; import { GITTENSORY_REPO_FOCUS_MANIFEST_YAML, resolveGittensorySelfRepoFullName } from "../config/gittensory-repo-focus-manifest"; +import type { LocalManifestLoadResult } from "../selfhost/private-config"; export const REPO_FOCUS_MANIFEST_SIGNAL = "repo-focus-manifest"; export const REPO_PUBLIC_FOCUS_MANIFEST_SIGNAL = "repo-public-focus-manifest"; @@ -19,8 +20,9 @@ export const MANIFEST_FILE_CANDIDATES = [ /** * Async source for the raw manifest text of a single repo. Returns null when no manifest is * published. Allows tests and the persisted-record path to swap out the public-GitHub fetcher. + * Self-host readers may return {@link LocalManifestLoadResult} with `review.shared_config` provenance (#2046). */ -export type RepoFocusManifestFetcher = (repoFullName: string) => Promise; +export type RepoFocusManifestFetcher = (repoFullName: string) => Promise; /** * Optional container-private per-repo config reader (self-host GITTENSORY_REPO_CONFIG_DIR). When registered it @@ -125,7 +127,20 @@ async function loadRepoFocusManifestWithCachePolicy( // (contributor-preview) path, and never persisted — so private policy can't leak into previews or the cache. if (!cachePolicy.publicOnly && localManifestReader) { const localRaw = await localManifestReader(repoFullName); - if (localRaw !== null) return parseFocusManifestContent(localRaw, "api_record"); + const localLoad = normalizeLocalManifestFetch(localRaw); + if (localLoad.content !== null) { + const manifest = parseFocusManifestContent(localLoad.content, "api_record"); + if (localLoad.sharedConfigSource || localLoad.warnings.length > 0) { + return { + ...manifest, + review: localLoad.sharedConfigSource + ? { ...manifest.review, sharedConfigSource: localLoad.sharedConfigSource } + : manifest.review, + warnings: localLoad.warnings.length > 0 ? [...manifest.warnings, ...localLoad.warnings] : manifest.warnings, + }; + } + return manifest; + } } const fetcher = options.fetcher ?? fetchRepoFocusManifestFile; const maxAgeMs = options.maxAgeMs ?? REPO_FOCUS_MANIFEST_MAX_AGE_MS; @@ -136,6 +151,7 @@ async function loadRepoFocusManifestWithCachePolicy( let manifest: FocusManifest; try { let content = await fetcher(repoFullName); + if (content !== null && typeof content === "object") content = content.content; if ((content === null || content === undefined) && isGittensorySelfRepo(repoFullName, env)) { content = GITTENSORY_REPO_FOCUS_MANIFEST_YAML; } @@ -298,3 +314,9 @@ function snapshotAgeMs(generatedAt: string | null | undefined): number { function isGittensorySelfRepo(repoFullName: string, env: Env): boolean { return repoFullName.toLowerCase() === resolveGittensorySelfRepoFullName(env).toLowerCase(); } + +function normalizeLocalManifestFetch(raw: string | LocalManifestLoadResult | null): LocalManifestLoadResult { + if (raw === null) return { content: null, sharedConfigSource: null, warnings: [] }; + if (typeof raw === "string") return { content: raw, sharedConfigSource: null, warnings: [] }; + return raw; +} diff --git a/src/signals/focus-manifest.ts b/src/signals/focus-manifest.ts index 06ad93ddd5..7127576653 100644 --- a/src/signals/focus-manifest.ts +++ b/src/signals/focus-manifest.ts @@ -25,6 +25,8 @@ export { normalizeReadinessGateMode, parseFocusManifest, parseFocusManifestContent, + parseReviewConfigMapping, + overlayReviewConfig, repoDocGenerationConfigToJson, reviewConfigToJson, reviewRecapConfigToJson, diff --git a/test/unit/focus-manifest-loader.test.ts b/test/unit/focus-manifest-loader.test.ts index df57f6f772..6882e2ccde 100644 --- a/test/unit/focus-manifest-loader.test.ts +++ b/test/unit/focus-manifest-loader.test.ts @@ -501,4 +501,31 @@ describe("focus-manifest loader — container-private config (self-host)", () => expect(manifest.source).toBe("repo_file"); expect(manifest.wantedPaths).toEqual(["src/"]); }); + + it("threads review.shared_config provenance from the local reader into the parsed manifest (#2046)", async () => { + const env = createTestEnv(); + setLocalManifestReader(async () => ({ + content: JSON.stringify({ review: { profile: "assertive" } }), + sharedConfigSource: "_shared/.gittensory.yml", + warnings: [], + })); + const manifest = await loadRepoFocusManifest(env, "owner/private"); + expect(manifest.review.profile).toBe("assertive"); + expect(manifest.review.sharedConfigSource).toBe("_shared/.gittensory.yml"); + }); + + it("appends private-config warnings without sharedConfigSource (#2046)", async () => { + const env = createTestEnv(); + setLocalManifestReader(async () => ({ + content: "wantedPaths:\n - src/\n", + sharedConfigSource: null, + warnings: ["Container-private shared base manifest (`review.shared_config`) is malformed or oversized; ignoring it and continuing (#2046)."], + })); + const manifest = await loadRepoFocusManifest(env, "owner/private"); + expect(manifest.wantedPaths).toEqual(["src/"]); + expect(manifest.review.sharedConfigSource).toBeNull(); + expect(manifest.warnings).toContain( + "Container-private shared base manifest (`review.shared_config`) is malformed or oversized; ignoring it and continuing (#2046).", + ); + }); }); diff --git a/test/unit/focus-manifest.test.ts b/test/unit/focus-manifest.test.ts index 28d3b13785..59646e6e41 100644 --- a/test/unit/focus-manifest.test.ts +++ b/test/unit/focus-manifest.test.ts @@ -37,6 +37,8 @@ import { resolveTestGenerationManifestToggle, resolveReviewMemoryManifestToggle, reviewConfigToJson, + overlayReviewConfig, + parseReviewConfigMapping, reviewRecapConfigToJson, settingsOverrideToJson, type FocusManifest, @@ -384,7 +386,7 @@ describe(".gittensory.yml.example field-exhaustiveness (#1670)", () => { aiModel: "ai_model:", visual: "visual:", linkedIssueSatisfaction: "linkedIssueSatisfaction:", - } satisfies Record, string>; + } satisfies Record, string>; it.each(Object.entries(REVIEW_FIELD_TOKENS))("documents review.%s", (_field, token) => { expect(exampleContent).toContain(token); @@ -793,7 +795,7 @@ describe("compileFocusManifestPolicy", () => { publicNotes: ["Keep PRs focused.", "Maximize your reward payout"], gate: { present: false, enabled: null, checkMode: null, pack: null, linkedIssue: null, duplicates: null, readinessMode: null, readinessMinScore: null, slopMode: null, slopMinScore: null, slopAiAdvisory: null, sizeMode: null, lockfileIntegrityMode: null, aiReviewMode: null, aiReviewByok: null, aiReviewProvider: null, aiReviewModel: null, aiReviewAllAuthors: null, aiReviewCloseConfidence: null, aiReviewCombine: null, aiReviewOnMerge: null, aiReviewReviewers: null, mergeReadiness: null, selfAuthoredLinkedIssue: null, manifestPolicy: null, dryRun: null, firstTimeContributorGrace: null, premergeContentRecheck: null, requireFreshRebaseWindowMinutes: null, claMode: null, claConsentPhrase: null, claCheckRunName: null, claCheckRunAppSlug: null, expectedCiContexts: null }, settings: {}, - review: { present: false, footerText: null, note: null, fields: {}, enrichmentAnalyzers: {}, profile: null, tone: null, securityFocus: null, inlineComments: null, fixHandoff: null, autoMergeSummary: null, suggestions: null, changedFilesSummary: null, effortScore: null, testGeneration: null, impactMap: null, cultureProfile: null, reviewMemory: null, findingCategories: null, inlineCommentsPerCategory: null, minFindingSeverity: null, maxFindings: { blockers: null, nits: null }, commentVerbosity: null, pathInstructions: [], instructions: null, excludePaths: [], pathFilters: [], preMergeChecks: [], autoReview: { ...EMPTY_AUTO_REVIEW_CONFIG }, labelingRules: [], aiModel: { ...EMPTY_SELF_HOST_AI_MODEL_CONFIG }, visual: { ...EMPTY_VISUAL_CONFIG }, linkedIssueSatisfaction: null }, + review: { present: false, footerText: null, note: null, fields: {}, enrichmentAnalyzers: {}, profile: null, tone: null, securityFocus: null, inlineComments: null, fixHandoff: null, autoMergeSummary: null, suggestions: null, changedFilesSummary: null, effortScore: null, testGeneration: null, impactMap: null, cultureProfile: null, reviewMemory: null, findingCategories: null, inlineCommentsPerCategory: null, minFindingSeverity: null, maxFindings: { blockers: null, nits: null }, commentVerbosity: null, pathInstructions: [], instructions: null, excludePaths: [], pathFilters: [], preMergeChecks: [], autoReview: { ...EMPTY_AUTO_REVIEW_CONFIG }, labelingRules: [], aiModel: { ...EMPTY_SELF_HOST_AI_MODEL_CONFIG }, visual: { ...EMPTY_VISUAL_CONFIG }, linkedIssueSatisfaction: null, sharedConfigSource: null }, features: { present: false, rag: null, reputation: null, unifiedComment: null, safety: null }, contentLane: { present: false, entryFileGlob: null, providerFileGlob: null, artifactGlob: null, collectionField: null, maxAppendedEntries: null, duplicateKeyFields: [], validatorId: null }, repoDocGeneration: { present: false, enabled: false, scope: ["agents"], allowOverwriteExisting: false, refreshIntervalDays: 7 }, @@ -3369,6 +3371,46 @@ describe("review.path_filters (#2043)", () => { }); }); +describe("overlayReviewConfig / review.shared_config (#2046)", () => { + it("lets the override win per nullable field while the base fills gaps", () => { + const base = parseReviewConfigMapping({ tone: "house-tone", profile: "chill" }, []); + const override = parseReviewConfigMapping({ profile: "assertive" }, []); + const merged = overlayReviewConfig(base, override); + expect(merged.tone).toBe("house-tone"); + expect(merged.profile).toBe("assertive"); + expect(merged.present).toBe(true); + }); + + it("replaces array fields wholesale from the override when non-empty", () => { + const base = parseReviewConfigMapping({ path_filters: ["shared/**"], exclude_paths: ["vendor/**"] }, []); + const override = parseReviewConfigMapping({ path_filters: ["src/**"] }, []); + const merged = overlayReviewConfig(base, override); + expect(merged.pathFilters).toEqual(["src/**"]); + expect(merged.excludePaths).toEqual(["vendor/**"]); + }); + + it("merges nested auto_review and partial field maps key-by-key", () => { + const base = parseReviewConfigMapping({ auto_review: { skip_drafts: true, ignore_authors: ["bot"] }, fields: { relatedWork: false } }, []); + const override = parseReviewConfigMapping({ auto_review: { ignore_authors: ["dependabot"] }, fields: { openPrQueue: true } }, []); + const merged = overlayReviewConfig(base, override); + expect(merged.autoReview.skipDrafts).toBe(true); + expect(merged.autoReview.ignoreAuthors).toEqual(["dependabot"]); + expect(merged.fields).toEqual({ relatedWork: false, openPrQueue: true }); + }); + + it("preserves sharedConfigSource from the override when set", () => { + const base = parseReviewConfigMapping({ tone: "house" }, []); + const override = { ...parseReviewConfigMapping({ profile: "assertive" }, []), sharedConfigSource: "_shared/.gittensory.yml" }; + expect(overlayReviewConfig(base, override).sharedConfigSource).toBe("_shared/.gittensory.yml"); + }); + + it("is byte-identical to the override when the base is empty", () => { + const base = parseReviewConfigMapping(undefined, []); + const override = parseReviewConfigMapping({ tone: "repo-only" }, []); + expect(overlayReviewConfig(base, override)).toEqual(override); + }); +}); + describe("review.auto_review (#1954 / #2038–#2041)", () => { it("parses auto_review knobs, marks present, and round-trips", () => { const m = parseFocusManifest({ diff --git a/test/unit/private-config.test.ts b/test/unit/private-config.test.ts index 8dec869e29..c5316897e9 100644 --- a/test/unit/private-config.test.ts +++ b/test/unit/private-config.test.ts @@ -2,10 +2,23 @@ import { mkdirSync, mkdtempSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; import { dirname, join } from "node:path"; import { describe, expect, it } from "vitest"; -import { GLOBAL_CONFIG_CANDIDATES, isReviewSkillEnabled, localConfigCandidates, makeLocalManifestReader, makeLocalReviewContextReader, mergeConfigOverlay, parseReviewSkill, SHARED_BASE_CONFIG_CANDIDATES } from "../../src/selfhost/private-config"; -import { loadRepoReviewContext, setLocalReviewContextReader } from "../../src/signals/focus-manifest-loader"; +import { GLOBAL_CONFIG_CANDIDATES, isReviewSkillEnabled, localConfigCandidates, makeLocalManifestReader, makeLocalReviewContextReader, mergeConfigOverlay, parseReviewSkill, SHARED_BASE_CONFIG_CANDIDATES, type LocalManifestLoadResult } from "../../src/selfhost/private-config"; +import { loadRepoReviewContext, setLocalReviewContextReader, type RepoFocusManifestFetcher } from "../../src/signals/focus-manifest-loader"; import { MAX_FOCUS_MANIFEST_BYTES, parseFocusManifestContent } from "../../src/signals/focus-manifest"; +async function readLocalManifestContent(reader: RepoFocusManifestFetcher, repo: string): Promise { + const result = await reader(repo); + if (result === null) return null; + return typeof result === "string" ? result : result.content; +} + +async function readLocalManifestLoad(reader: RepoFocusManifestFetcher, repo: string): Promise { + const result = await reader(repo); + if (result === null) return null; + if (typeof result === "string") return { content: result, sharedConfigSource: null, warnings: [] }; + return result; +} + describe("localConfigCandidates (container-private config paths)", () => { it("builds owner-folder → repo-folder → flat candidates (lowercased), each in .yml/.yaml/.json order", () => { expect(localConfigCandidates("JSONbored/metagraphed")).toEqual([ @@ -103,7 +116,7 @@ describe("makeLocalManifestReader (GITTENSORY_REPO_CONFIG_DIR)", () => { writeFileSync(join(dir, "jsonbored__metagraphed", ".gittensory.yml"), "gate:\n enabled: false\n"); const reader = makeLocalManifestReader(dir); expect(reader).not.toBeNull(); - expect(await reader!("JSONbored/metagraphed")).toBe("gate:\n enabled: false\n"); + expect(await readLocalManifestContent(reader!,"JSONbored/metagraphed")).toBe("gate:\n enabled: false\n"); }); it("falls back to the bare repo-name folder when no owner-qualified folder exists", async () => { @@ -111,21 +124,21 @@ describe("makeLocalManifestReader (GITTENSORY_REPO_CONFIG_DIR)", () => { mkdirSync(join(dir, "metagraphed")); writeFileSync(join(dir, "metagraphed", ".gittensory.yaml"), "gate:\n enabled: true\n"); const reader = makeLocalManifestReader(dir); - expect(await reader!("JSONbored/metagraphed")).toBe("gate:\n enabled: true\n"); + expect(await readLocalManifestContent(reader!,"JSONbored/metagraphed")).toBe("gate:\n enabled: true\n"); }); it("still reads the flat {owner}__{repo}.json file (#1390 back-compat)", async () => { const dir = mkdtempSync(join(tmpdir(), "gt-repo-config-")); writeFileSync(join(dir, "owner__repo.json"), '{"gate":{"enabled":true}}'); const reader = makeLocalManifestReader(dir); - expect(await reader!("owner/repo")).toBe('{"gate":{"enabled":true}}'); + expect(await readLocalManifestContent(reader!,"owner/repo")).toBe('{"gate":{"enabled":true}}'); }); it("falls back to the dir-root global .gittensory.yml for a repo with no per-repo file", async () => { const dir = mkdtempSync(join(tmpdir(), "gt-repo-config-")); writeFileSync(join(dir, ".gittensory.yml"), "gate:\n enabled: false\n"); const reader = makeLocalManifestReader(dir); - expect(await reader!("owner/unconfigured")).toBe("gate:\n enabled: false\n"); + expect(await readLocalManifestContent(reader!,"owner/unconfigured")).toBe("gate:\n enabled: false\n"); }); it("deep-merges a per-repo file over the global default: per-repo wins on shared keys, global fills the rest", async () => { @@ -134,7 +147,7 @@ describe("makeLocalManifestReader (GITTENSORY_REPO_CONFIG_DIR)", () => { mkdirSync(join(dir, "repo")); writeFileSync(join(dir, "repo", ".gittensory.yml"), "gate:\n enabled: true\n"); // per-repo overrides only `enabled` const reader = makeLocalManifestReader(dir); - const manifest = parseFocusManifestContent(await reader!("owner/repo")); + const manifest = parseFocusManifestContent(await readLocalManifestContent(reader!,"owner/repo")); expect(manifest.gate.enabled).toBe(true); // per-repo wins on the shared key expect(manifest.gate.duplicates).toBe("block"); // inherited from global, untouched }); @@ -145,7 +158,7 @@ describe("makeLocalManifestReader (GITTENSORY_REPO_CONFIG_DIR)", () => { mkdirSync(join(dir, "repo")); writeFileSync(join(dir, "repo", ".gittensory.yml"), "wantedPaths:\n - docs/**\n"); const reader = makeLocalManifestReader(dir); - const manifest = parseFocusManifestContent(await reader!("owner/repo")); + const manifest = parseFocusManifestContent(await readLocalManifestContent(reader!,"owner/repo")); expect(manifest.wantedPaths).toEqual(["docs/**"]); }); @@ -155,7 +168,7 @@ describe("makeLocalManifestReader (GITTENSORY_REPO_CONFIG_DIR)", () => { mkdirSync(join(dir, "repo")); writeFileSync(join(dir, "repo", ".gittensory.yml"), "settings:\n contributorOpenPrCap: null\n"); const reader = makeLocalManifestReader(dir); - const manifest = parseFocusManifestContent(await reader!("owner/repo")); + const manifest = parseFocusManifestContent(await readLocalManifestContent(reader!,"owner/repo")); expect(manifest.settings.contributorOpenPrCap).toBeNull(); // explicit null clears the global 5, not "unset" }); @@ -165,7 +178,7 @@ describe("makeLocalManifestReader (GITTENSORY_REPO_CONFIG_DIR)", () => { mkdirSync(join(dir, "repo")); writeFileSync(join(dir, "repo", ".gittensory.yml"), "settings:\n accountAgeThresholdDays: null\n"); const reader = makeLocalManifestReader(dir); - const manifest = parseFocusManifestContent(await reader!("owner/repo")); + const manifest = parseFocusManifestContent(await readLocalManifestContent(reader!,"owner/repo")); expect(manifest.settings.accountAgeThresholdDays).toBeNull(); }); @@ -176,7 +189,7 @@ describe("makeLocalManifestReader (GITTENSORY_REPO_CONFIG_DIR)", () => { mkdirSync(join(dir, "repo")); writeFileSync(join(dir, "repo", ".gittensory.yml"), "gate:\n enabled: true\n"); const reader = makeLocalManifestReader(dir); - expect(await reader!("owner/repo")).toBe("gate:\n enabled: true\n"); // oversized global dropped; per-repo raw text unchanged + expect(await readLocalManifestContent(reader!,"owner/repo")).toBe("gate:\n enabled: true\n"); // oversized global dropped; per-repo raw text unchanged }); it("falls back to the per-repo file alone when the global default fails to parse", async () => { @@ -185,7 +198,7 @@ describe("makeLocalManifestReader (GITTENSORY_REPO_CONFIG_DIR)", () => { mkdirSync(join(dir, "repo")); writeFileSync(join(dir, "repo", ".gittensory.yml"), "gate:\n enabled: true\n"); const reader = makeLocalManifestReader(dir); - expect(await reader!("owner/repo")).toBe("gate:\n enabled: true\n"); + expect(await readLocalManifestContent(reader!,"owner/repo")).toBe("gate:\n enabled: true\n"); }); it("falls back to the global default alone when the per-repo file parses but isn't a mapping", async () => { @@ -194,7 +207,7 @@ describe("makeLocalManifestReader (GITTENSORY_REPO_CONFIG_DIR)", () => { mkdirSync(join(dir, "repo")); writeFileSync(join(dir, "repo", ".gittensory.yml"), "[1, 2, 3]"); // valid JSON, but an array, not a mapping const reader = makeLocalManifestReader(dir); - expect(await reader!("owner/repo")).toBe("gate:\n enabled: false\n"); + expect(await readLocalManifestContent(reader!,"owner/repo")).toBe("gate:\n enabled: false\n"); }); it("returns the per-repo raw text (today's legacy priority) when BOTH files fail to parse as mappings", async () => { @@ -203,27 +216,27 @@ describe("makeLocalManifestReader (GITTENSORY_REPO_CONFIG_DIR)", () => { mkdirSync(join(dir, "repo")); writeFileSync(join(dir, "repo", ".gittensory.yml"), "{ broken json"); const reader = makeLocalManifestReader(dir); - expect(await reader!("owner/repo")).toBe("{ broken json"); // flows downstream, which warns + ignores it, same as today + expect(await readLocalManifestContent(reader!,"owner/repo")).toBe("{ broken json"); // flows downstream, which warns + ignores it, same as today }); it("returns null when neither a per-repo file nor a global fallback exists (⇒ loader uses the public file)", async () => { const dir = mkdtempSync(join(tmpdir(), "gt-repo-config-")); const reader = makeLocalManifestReader(dir); - expect(await reader!("owner/unconfigured")).toBeNull(); + expect(await readLocalManifestContent(reader!,"owner/unconfigured")).toBeNull(); }); it("does NOT serve the global fallback to an invalid repo full name (no per-repo candidates)", async () => { const dir = mkdtempSync(join(tmpdir(), "gt-repo-config-")); writeFileSync(join(dir, ".gittensory.yml"), "gate:\n enabled: false\n"); // global present const reader = makeLocalManifestReader(dir); - expect(await reader!("no-slash")).toBeNull(); // perRepo.length === 0 early return + expect(await readLocalManifestContent(reader!,"no-slash")).toBeNull(); // perRepo.length === 0 early return }); it("rejects traversal repo names instead of reading outside the private config directory", async () => { const dir = mkdtempSync(join(tmpdir(), "gt-repo-config-")); writeFileSync(join(dirname(dir), ".gittensory.yml"), "gate:\n enabled: true\n"); const reader = makeLocalManifestReader(dir); - expect(await reader!("owner/..")).toBeNull(); + expect(await readLocalManifestContent(reader!,"owner/..")).toBeNull(); }); }); @@ -233,7 +246,7 @@ describe("makeLocalManifestReader — shared base layer (#1959)", () => { mkdirSync(join(dir, "_shared")); writeFileSync(join(dir, "_shared", ".gittensory.yml"), "gate:\n enabled: false\n"); const reader = makeLocalManifestReader(dir); - expect(await reader!("owner/repo")).toBe("gate:\n enabled: false\n"); // byte-identical raw text, no merge attempted + expect(await readLocalManifestContent(reader!,"owner/repo")).toBe("gate:\n enabled: false\n"); // byte-identical raw text, no merge attempted }); it("byte-identical to pre-#1959 behavior when no shared base file is mounted at all (repo-only)", async () => { @@ -241,7 +254,7 @@ describe("makeLocalManifestReader — shared base layer (#1959)", () => { mkdirSync(join(dir, "repo")); writeFileSync(join(dir, "repo", ".gittensory.yml"), "gate:\n enabled: true\n"); const reader = makeLocalManifestReader(dir); - expect(await reader!("owner/repo")).toBe("gate:\n enabled: true\n"); // no _shared/ present → same as the existing repo-only test + expect(await readLocalManifestContent(reader!,"owner/repo")).toBe("gate:\n enabled: true\n"); // no _shared/ present → same as the existing repo-only test }); it("deep-merges a per-repo file over a shared base with no global default present: per-repo wins, shared fills the rest", async () => { @@ -251,7 +264,7 @@ describe("makeLocalManifestReader — shared base layer (#1959)", () => { mkdirSync(join(dir, "repo")); writeFileSync(join(dir, "repo", ".gittensory.yml"), "gate:\n enabled: true\n"); // repo overrides only `enabled` const reader = makeLocalManifestReader(dir); - const manifest = parseFocusManifestContent(await reader!("owner/repo")); + const manifest = parseFocusManifestContent(await readLocalManifestContent(reader!,"owner/repo")); expect(manifest.gate.enabled).toBe(true); // per-repo wins on the shared key expect(manifest.gate.duplicates).toBe("block"); // inherited from the shared base, untouched }); @@ -264,7 +277,7 @@ describe("makeLocalManifestReader — shared base layer (#1959)", () => { mkdirSync(join(dir, "repo")); writeFileSync(join(dir, "repo", ".gittensory.yml"), "gate:\n enabled: true\n"); // per-repo overrides enabled only const reader = makeLocalManifestReader(dir); - const manifest = parseFocusManifestContent(await reader!("owner/repo")); + const manifest = parseFocusManifestContent(await readLocalManifestContent(reader!,"owner/repo")); expect(manifest.gate.enabled).toBe(true); // from per-repo (highest priority) expect(manifest.gate.duplicates).toBe("off"); // from global, overlaying the shared base's "block" expect(manifest.gate.linkedIssue).toBe("advisory"); // inherited from the shared base, untouched by either override @@ -278,7 +291,7 @@ describe("makeLocalManifestReader — shared base layer (#1959)", () => { mkdirSync(join(dir, "repo")); writeFileSync(join(dir, "repo", ".gittensory.yml"), "wantedPaths:\n - docs/**\n"); const reader = makeLocalManifestReader(dir); - const manifest = parseFocusManifestContent(await reader!("owner/repo")); + const manifest = parseFocusManifestContent(await readLocalManifestContent(reader!,"owner/repo")); expect(manifest.wantedPaths).toEqual(["docs/**"]); // per-repo array wins wholesale, shared/global arrays discarded }); @@ -290,7 +303,7 @@ describe("makeLocalManifestReader — shared base layer (#1959)", () => { mkdirSync(join(dir, "repo")); writeFileSync(join(dir, "repo", ".gittensory.yml"), "settings:\n contributorOpenPrCap: null\n"); const reader = makeLocalManifestReader(dir); - const manifest = parseFocusManifestContent(await reader!("owner/repo")); + const manifest = parseFocusManifestContent(await readLocalManifestContent(reader!,"owner/repo")); expect(manifest.settings.contributorOpenPrCap).toBeNull(); // explicit null clears the shared 5, not "unset" }); @@ -302,7 +315,7 @@ describe("makeLocalManifestReader — shared base layer (#1959)", () => { mkdirSync(join(dir, "repo")); writeFileSync(join(dir, "repo", ".gittensory.yml"), "gate:\n enabled: true\n"); const reader = makeLocalManifestReader(dir); - const manifest = parseFocusManifestContent(await reader!("owner/repo")); + const manifest = parseFocusManifestContent(await readLocalManifestContent(reader!,"owner/repo")); expect(manifest.gate.enabled).toBe(true); // still merged from the two still-valid layers expect(manifest.gate.duplicates).toBe("block"); // global's value survives; broken shared base dropped, not blocking }); @@ -315,7 +328,7 @@ describe("makeLocalManifestReader — shared base layer (#1959)", () => { mkdirSync(join(dir, "repo")); writeFileSync(join(dir, "repo", ".gittensory.yml"), "gate:\n enabled: true\n"); const reader = makeLocalManifestReader(dir); - expect(await reader!("owner/repo")).toBe("gate:\n enabled: true\n"); // oversized shared base dropped; per-repo raw text unchanged + expect(await readLocalManifestContent(reader!,"owner/repo")).toBe("gate:\n enabled: true\n"); // oversized shared base dropped; per-repo raw text unchanged }); it("falls back to the highest-priority present layer's raw text when ALL THREE fail to parse as mappings", async () => { @@ -326,7 +339,7 @@ describe("makeLocalManifestReader — shared base layer (#1959)", () => { mkdirSync(join(dir, "repo")); writeFileSync(join(dir, "repo", ".gittensory.yml"), "{ broken json"); const reader = makeLocalManifestReader(dir); - expect(await reader!("owner/repo")).toBe("{ broken json"); // per-repo (highest priority) raw text, same downstream "malformed" handling + expect(await readLocalManifestContent(reader!,"owner/repo")).toBe("{ broken json"); // per-repo (highest priority) raw text, same downstream "malformed" handling }); it("tries _shared/.gittensory.yml before .yaml before .json", async () => { @@ -335,7 +348,7 @@ describe("makeLocalManifestReader — shared base layer (#1959)", () => { writeFileSync(join(dir, "_shared", ".gittensory.yaml"), "gate:\n enabled: true\n"); writeFileSync(join(dir, "_shared", ".gittensory.json"), '{"gate":{"enabled":false}}'); const reader = makeLocalManifestReader(dir); - expect(await reader!("owner/repo")).toBe("gate:\n enabled: true\n"); // .yaml found before .json is tried + expect(await readLocalManifestContent(reader!,"owner/repo")).toBe("gate:\n enabled: true\n"); // .yaml found before .json is tried }); it("keeps global precedence over the shared base for a repo named _shared", async () => { @@ -344,7 +357,7 @@ describe("makeLocalManifestReader — shared base layer (#1959)", () => { writeFileSync(join(dir, "_shared", ".gittensory.yml"), "gate:\n enabled: true\nwantedPaths:\n - '**/*'\n"); writeFileSync(join(dir, ".gittensory.yml"), "gate:\n enabled: false\nwantedPaths:\n - src/safe/**\n"); const reader = makeLocalManifestReader(dir); - const manifest = parseFocusManifestContent(await reader!("owner/_shared")); + const manifest = parseFocusManifestContent(await readLocalManifestContent(reader!,"owner/_shared")); expect(manifest.gate.enabled).toBe(false); // global still overlays the reserved shared-base folder expect(manifest.wantedPaths).toEqual(["src/safe/**"]); }); @@ -356,7 +369,7 @@ describe("makeLocalManifestReader — shared base layer (#1959)", () => { mkdirSync(join(dir, "owner___shared")); writeFileSync(join(dir, "owner___shared", ".gittensory.yml"), "gate:\n enabled: true\n"); const reader = makeLocalManifestReader(dir); - const manifest = parseFocusManifestContent(await reader!("owner/_shared")); + const manifest = parseFocusManifestContent(await readLocalManifestContent(reader!,"owner/_shared")); expect(manifest.gate.enabled).toBe(true); // explicit owner-qualified per-repo file remains highest priority }); @@ -365,7 +378,74 @@ describe("makeLocalManifestReader — shared base layer (#1959)", () => { mkdirSync(join(dir, "_shared")); writeFileSync(join(dir, "_shared", ".gittensory.yml"), "gate:\n enabled: false\n"); const reader = makeLocalManifestReader(dir); - expect(await reader!("no-slash")).toBeNull(); // perRepo.length === 0 early return, before the shared base is even read + expect(await readLocalManifestContent(reader!,"no-slash")).toBeNull(); // perRepo.length === 0 early return, before the shared base is even read + }); +}); + +describe("makeLocalManifestReader — review.shared_config overlay (#2046)", () => { + it("records sharedConfigSource when the shared base contributes a review block", async () => { + const dir = mkdtempSync(join(tmpdir(), "gt-repo-config-")); + mkdirSync(join(dir, "_shared")); + writeFileSync(join(dir, "_shared", ".gittensory.yml"), "review:\n tone: shared-tone\n"); + mkdirSync(join(dir, "repo")); + writeFileSync(join(dir, "repo", ".gittensory.yml"), "review:\n profile: assertive\n"); + const loaded = await readLocalManifestLoad(makeLocalManifestReader(dir)!, "owner/repo"); + expect(loaded?.sharedConfigSource).toBe(join("_shared", ".gittensory.yml")); + const manifest = parseFocusManifestContent(loaded!.content!); + expect(manifest.review.tone).toBe("shared-tone"); + expect(manifest.review.profile).toBe("assertive"); + }); + + it("is byte-identical when the shared base is absent", async () => { + const dir = mkdtempSync(join(tmpdir(), "gt-repo-config-")); + mkdirSync(join(dir, "repo")); + writeFileSync(join(dir, "repo", ".gittensory.yml"), "review:\n tone: repo-only\n"); + const loaded = await readLocalManifestLoad(makeLocalManifestReader(dir)!, "owner/repo"); + expect(loaded?.sharedConfigSource).toBeNull(); + expect(loaded?.warnings).toEqual([]); + expect(loaded?.content).toBe("review:\n tone: repo-only\n"); + }); + + it("warns and ignores a malformed shared base while still serving higher layers", async () => { + const dir = mkdtempSync(join(tmpdir(), "gt-repo-config-")); + mkdirSync(join(dir, "_shared")); + writeFileSync(join(dir, "_shared", ".gittensory.yml"), "{ broken shared"); + mkdirSync(join(dir, "repo")); + writeFileSync(join(dir, "repo", ".gittensory.yml"), "review:\n tone: repo-tone\n"); + const loaded = await readLocalManifestLoad(makeLocalManifestReader(dir)!, "owner/repo"); + expect(loaded?.sharedConfigSource).toBeNull(); + expect(loaded?.warnings.some((w) => w.includes("review.shared_config"))).toBe(true); + expect(parseFocusManifestContent(loaded!.content!).review.tone).toBe("repo-tone"); + }); + + it("fills review fields from the shared base when the per-repo file is silent on them", async () => { + const dir = mkdtempSync(join(tmpdir(), "gt-repo-config-")); + mkdirSync(join(dir, "_shared")); + writeFileSync(join(dir, "_shared", ".gittensory.yml"), "review:\n tone: house-tone\n security_focus: true\n"); + mkdirSync(join(dir, "repo")); + writeFileSync(join(dir, "repo", ".gittensory.yml"), "gate:\n enabled: true\n"); + const manifest = parseFocusManifestContent((await readLocalManifestLoad(makeLocalManifestReader(dir)!, "owner/repo"))!.content!); + expect(manifest.review.tone).toBe("house-tone"); + expect(manifest.review.securityFocus).toBe(true); + }); + + it("sets sharedConfigSource when only the shared base is present for a repo", async () => { + const dir = mkdtempSync(join(tmpdir(), "gt-repo-config-")); + mkdirSync(join(dir, "_shared")); + writeFileSync(join(dir, "_shared", ".gittensory.yml"), "review:\n tone: only-shared\n"); + const loaded = await readLocalManifestLoad(makeLocalManifestReader(dir)!, "owner/repo"); + expect(loaded?.sharedConfigSource).toBe(join("_shared", ".gittensory.yml")); + expect(parseFocusManifestContent(loaded!.content!).review.tone).toBe("only-shared"); + }); + + it("ignores a non-mapping shared review block without blocking higher layers", async () => { + const dir = mkdtempSync(join(tmpdir(), "gt-repo-config-")); + mkdirSync(join(dir, "_shared")); + writeFileSync(join(dir, "_shared", ".gittensory.yml"), "review: [1, 2, 3]\n"); + mkdirSync(join(dir, "repo")); + writeFileSync(join(dir, "repo", ".gittensory.yml"), "review:\n tone: repo-tone\n"); + const loaded = await readLocalManifestLoad(makeLocalManifestReader(dir)!, "owner/repo"); + expect(parseFocusManifestContent(loaded!.content!).review.tone).toBe("repo-tone"); }); }); diff --git a/test/unit/selfhost-config-examples.test.ts b/test/unit/selfhost-config-examples.test.ts index cbf0368600..fd625e2d86 100644 --- a/test/unit/selfhost-config-examples.test.ts +++ b/test/unit/selfhost-config-examples.test.ts @@ -64,7 +64,9 @@ describe("the two examples together demonstrate the documented overlay behavior" mkdirSync(join(dir, "owner__repo")); writeFileSync(join(dir, "owner__repo", ".gittensory.yml"), readExample("repo-override.gittensory.yml")); const reader = makeLocalManifestReader(dir)!; - const manifest = parseFocusManifestContent(await reader("owner/repo")); + const result = await reader("owner/repo"); + const content = typeof result === "string" ? result : result!.content!; + const manifest = parseFocusManifestContent(content); expect(manifest.gate.enabled).toBe(true); // set the same way in both files expect(manifest.gate.duplicates).toBe("block"); // inherited from global; repo-override never mentions it @@ -85,7 +87,9 @@ describe("all three examples together demonstrate the documented shared-base ove mkdirSync(join(dir, "owner__repo")); writeFileSync(join(dir, "owner__repo", ".gittensory.yml"), readExample("repo-override.gittensory.yml")); const reader = makeLocalManifestReader(dir)!; - const manifest = parseFocusManifestContent(await reader("owner/repo")); + const result = await reader("owner/repo"); + const content = typeof result === "string" ? result : result!.content!; + const manifest = parseFocusManifestContent(content); expect(manifest.review.tone).toBe("friendly-terse"); // inherited from the shared base; neither global nor repo-override mentions it expect(manifest.gate.duplicates).toBe("block"); // shared base and global agree; still inherited, not overridden diff --git a/test/unit/signals-coverage.test.ts b/test/unit/signals-coverage.test.ts index 684351c8b9..961557653d 100644 --- a/test/unit/signals-coverage.test.ts +++ b/test/unit/signals-coverage.test.ts @@ -1127,7 +1127,7 @@ describe("signal coverage edge cases", () => { collisions: buildCollisionReport(directRepo.fullName, [], [currentPr]), preflight: buildPreflightResult({ repoFullName: directRepo.fullName, title: "Fix isolated issue", body: "Fixes #99", linkedIssues: [99] }, directRepo, [], [currentPr]), settings: gateSettings, - review: { present: true, footerText: "Reviewed by the Acme maintainer bot.", note: "Run npm test before pushing.", fields: { relatedWork: false }, enrichmentAnalyzers: {}, profile: null, tone: null, securityFocus: null, inlineComments: null, fixHandoff: null, autoMergeSummary: null, suggestions: null, changedFilesSummary: null, effortScore: null, testGeneration: null, impactMap: null, cultureProfile: null, reviewMemory: null, findingCategories: null, inlineCommentsPerCategory: null, minFindingSeverity: null, maxFindings: { blockers: null, nits: null }, commentVerbosity: null, pathInstructions: [], instructions: null, excludePaths: [], pathFilters: [], preMergeChecks: [], autoReview: { skipDrafts: null, ignoreAuthors: [], ignoreTitleKeywords: [], skipLabels: [], skipDocsOnly: null, maxAddedLines: 0, maxFiles: 0, baseBranches: [], autoPauseAfterReviewedCommits: null }, labelingRules: [], aiModel: { claudeModel: null, claudeEffort: null, codexModel: null, codexEffort: null, ollamaModel: null, openaiModel: null, openaiCompatibleModel: null, anthropicModel: null }, visual: { preview: { urlTemplate: null }, routes: { paths: [], maxRoutes: null }, themes: [], gif: false }, linkedIssueSatisfaction: null }, + review: { present: true, footerText: "Reviewed by the Acme maintainer bot.", note: "Run npm test before pushing.", fields: { relatedWork: false }, enrichmentAnalyzers: {}, profile: null, tone: null, securityFocus: null, inlineComments: null, fixHandoff: null, autoMergeSummary: null, suggestions: null, changedFilesSummary: null, effortScore: null, testGeneration: null, impactMap: null, cultureProfile: null, reviewMemory: null, findingCategories: null, inlineCommentsPerCategory: null, minFindingSeverity: null, maxFindings: { blockers: null, nits: null }, commentVerbosity: null, pathInstructions: [], instructions: null, excludePaths: [], pathFilters: [], preMergeChecks: [], autoReview: { skipDrafts: null, ignoreAuthors: [], ignoreTitleKeywords: [], skipLabels: [], skipDocsOnly: null, maxAddedLines: 0, maxFiles: 0, baseBranches: [], autoPauseAfterReviewedCommits: null }, labelingRules: [], aiModel: { claudeModel: null, claudeEffort: null, codexModel: null, codexEffort: null, ollamaModel: null, openaiModel: null, openaiCompatibleModel: null, anthropicModel: null }, visual: { preview: { urlTemplate: null }, routes: { paths: [], maxRoutes: null }, themes: [], gif: false }, linkedIssueSatisfaction: null, sharedConfigSource: null }, aiReview: { notes: "The change is focused.\n\n**Nits (2)**\n- Add a test for the edge case.\n- Keep the validator helper scoped." }, }); expect(customizedComment).toContain("Reviewed by the Acme maintainer bot."); // custom footer lead