Configure twin-server change-copy ignores - #162
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| export const CopyIgnoreConfigSchema = z.object({ | ||
| /** Repository-relative directory patterns excluded from change copying. */ | ||
| folders: z.array(z.string().min(1)).optional(), | ||
| /** Repository-relative file patterns excluded from change copying. */ | ||
| files: z.array(z.string().min(1)).optional(), | ||
| }); |
There was a problem hiding this comment.
CopyIgnoreConfigSchema doesn't call .strict(), unlike SetupCommandSchema (line 15) and TwinServersConfigSchema (line 65) in this same file. Zod's default object mode silently strips unrecognized keys instead of failing validation, so a typo like copyIgnore: { folder: [...] } (missing the trailing s) passes schema validation with the override silently dropped. resolveConfig (config.ts) then merges validConfig.copyIgnore (now effectively {}) over the packaged defaults, so the user's intended list is silently replaced by compare-results/compare-bisect-results with no error or warning anywhere. Recommend adding .strict() here to match this file's existing convention and surface typos.
| @@ -943,6 +945,7 @@ export async function runServersMenu( | |||
| let deleted = 0; | |||
| const errors: Error[] = []; | |||
| for (const rel of batch) { | |||
| if (isCopyIgnored(copyIgnore, rel)) continue; | |||
There was a problem hiding this comment.
README-twin-servers.md documents copyIgnore patterns as "relative to the Git repository root," and that's what getChangedFiles does for sync-changes/copy-changes-to-ssh (both resolve sourceDir via getGitRootDirectory before filtering). Here, though, copyIgnore is matched (line 948) against rel, which comes from fs.watch(experimentBuildDir, ...) further down (~line 1035) — i.e. paths relative to the Docker build context (config.dockerBuildDir), not the git root.
When dockerBuildDir isn't the git root — which the twin-servers setup guide's own reference config achieves via dockerBuildDir: '..', and deeper monorepo nesting is explicitly supported — a slash-containing pattern like the README's own example (tmp/traces) gets anchored to a different base by this live auto-sync path than by the other two copy mechanisms, so identical config filters files inconsistently depending on which sync path runs. The shipped defaults (compare-results, compare-bisect-results) are unaffected since gitignore matches slash-less names at any depth, but this will silently misbehave for anyone adding a custom, path-qualified copyIgnore entry in a project whose build context isn't the repo root.
Review summaryReviewed the
No security concerns — this only affects local file-copy filtering driven by a trusted local config file, no untrusted/network input involved. Test coverage for |
| let deleted = 0; | ||
| const errors: Error[] = []; | ||
| for (const rel of batch) { | ||
| if (isCopyIgnored(copyIgnore, rel)) continue; |
There was a problem hiding this comment.
Bug: copyIgnore is matched against the wrong root here, so multi-segment overrides silently no-op in live auto-sync.
rel comes from fs.watch(experimentBuildDir, …) (line 1035), so it's relative to dockerBuildDir (line 925: experimentBuildDir = dockerBuildDirForSide(config, 'experiment')). But README-twin-servers.md's new copy-ignore section, and both other call sites (sync-changes.ts, copy-changes-to-ssh.ts), define/resolve copyIgnore patterns as relative to the git repository root — sync-changes.ts explicitly does getGitRootDirectory(sideBuildDir) before calling getChangedFiles.
dockerBuildDir is not guaranteed to equal the git root — this PR's own commands.test.ts test "maps git-root paths into a nested build context" exercises exactly that divergence (dockerBuildDir nested under the repo root), and the sibling helpers/dockerignore.ts's findUnignoredShakaResultDirs already has to prefix patterns to compensate for this same gap.
Concretely: with a nested build dir, a user-configured copyIgnore.folders: ['packages/web/tmp/traces'] (git-root-relative, per the docs) would correctly exclude that folder for sync-changes/copy-changes-to-ssh, but silently fail to match here, because rel for that path would arrive as tmp/traces/... (no packages/web/ prefix). It only happens to work for the default patterns (compare-results, compare-bisect-results) because those are single-segment and gitignore-style patterns without a / match at any depth regardless of root — which is likely why no test caught this.
Worth resolving rel against the git root (or prefixing patterns the way findUnignoredShakaResultDirs does) before matching against copyIgnore here.
| ] as const; | ||
|
|
||
| /** Host-only files excluded from change copying unless config overrides them. */ | ||
| export const DEFAULT_COPY_IGNORE_FILES: readonly string[] = []; |
There was a problem hiding this comment.
Per this repo's CLAUDE.md ("Breaking changes"): any change that can break an existing consumer's abtests.config.ts behavior — including "a changed default" — must get an Unreleased entry in BREAKING_CHANGES.md. Before this PR, getChangedFiles/sync-changes/copy-changes-to-ssh copied every git-changed file to the twin-server containers; after this PR, compare-results/ and compare-bisect-results/ are silently excluded by default, with no config change required from existing users. That's a new default behavior change (even if it's a strict improvement), so it looks like it should get a BREAKING_CHANGES.md Unreleased entry rather than being introduced silently.
Review summarySolid feature overall — the Left two inline comments:
No security or other correctness issues found; the |
| @@ -943,6 +945,7 @@ export async function runServersMenu( | |||
| let deleted = 0; | |||
| const errors: Error[] = []; | |||
| for (const rel of batch) { | |||
| if (isCopyIgnored(copyIgnore, rel)) continue; | |||
There was a problem hiding this comment.
Bug: copyIgnore is matched against the wrong path base here, breaking anchored (slash-containing) override patterns in the auto-sync path.
rel comes from fs.watch(experimentBuildDir, ...) (line 1035-1037), so it's relative to the Docker build context (dockerBuildDirForSide). But config.copyIgnore is documented — and used everywhere else — as relative to the Git repository root:
sync-changes.tsfiltersgetChangedFiles(sourceDir, config.copyIgnore)wheresourceDir = getGitRootDirectory(sideBuildDir).copy-changes-to-ssh.tsdoes the same viagetGitRootDirectory(config.dockerBuildDir).- README-twin-servers.md explicitly says: "Manual sync, running-menu auto-sync, and
copy-changes-to-sshalways leaveaudit-results/... Paths use gitignore pattern syntax and are relative to the Git repository root."
sync-changes.ts even has a comment acknowledging the two bases differ: "Git reports paths from the repository root, while the bind mount and build manifest are relative to the Docker build context."
For the default unslashed folder names (audit-results, compare-results, compare-bisect-results) this happens to work regardless of base, because unanchored gitignore patterns match at any depth. But for a project override containing a path separator — exactly the example given in this PR's own README and abtests.config.ts template ('tmp/traces') — gitignore anchoring semantics mean the pattern only matches paths that literally start with that prefix. Whenever the Docker build context isn't identical to the git root (e.g. dockerBuildDir: '.', the template's default, in a repo where the project isn't itself the git root), a tmp/traces override will correctly exclude files in sync-changes/copy-changes-to-ssh but silently fail to exclude them in the live running-menu auto-sync loop — the exact scenario this PR is meant to prevent.
There's no test exercising this path (no test file for helpers/copy-ignore.ts, and no servers-menu test covers performAutoSync's copyIgnore filtering), so this regressed silently.
Fix: derive rel's path relative to the git root (or re-anchor the copyIgnore patterns to the build-context base) before calling isCopyIgnored, the same way sync-changes.ts reconciles sourceDir vs sideBuildDir.
|
Reviewed the diff. Overall the feature is clean: schema/defaults/merge logic in One real bug, flagged inline on Everything else (types, docs, test updates for the default/override merge behavior) looks correct and matches the stated intent. |
| export const CopyIgnoreConfigSchema = z.object({ | ||
| /** Repository-relative directory patterns excluded from change copying. */ | ||
| folders: z.array(z.string().min(1)).optional(), | ||
| /** Repository-relative file patterns excluded from change copying. */ | ||
| files: z.array(z.string().min(1)).optional(), | ||
| }); |
There was a problem hiding this comment.
CopyIgnoreConfigSchema is missing .strict(), unlike every other schema in this file (SetupCommandSchema, TwinServersConfigSchema). Because a plain z.object() defaults to "strip unknown keys" rather than erroring, a typo in a user's abtests.config.ts — e.g. copyIgnore: { folder: [...] } instead of folders — silently parses successfully and falls back to the packaged defaults. The user gets no error and no indication their override was ignored; they'll only notice when unexpected files show up in (or stay out of) the synced volume.
Suggest adding .strict() here to match the rest of the file's convention:
| export const CopyIgnoreConfigSchema = z.object({ | |
| /** Repository-relative directory patterns excluded from change copying. */ | |
| folders: z.array(z.string().min(1)).optional(), | |
| /** Repository-relative file patterns excluded from change copying. */ | |
| files: z.array(z.string().min(1)).optional(), | |
| }); | |
| export const CopyIgnoreConfigSchema = z.object({ | |
| /** Repository-relative directory patterns excluded from change copying. */ | |
| folders: z.array(z.string().min(1)).optional(), | |
| /** Repository-relative file patterns excluded from change copying. */ | |
| files: z.array(z.string().min(1)).optional(), | |
| }).strict(); |
Review summaryReviewed the copy-ignore feature for correctness across its three call sites ( What looks solid:
Issue found (left as inline comment):
Minor note (not blocking): this PR changes the default behavior of |
| export interface CopyIgnoreConfig { | ||
| folders: string[]; | ||
| files: string[]; | ||
| } |
There was a problem hiding this comment.
CopyIgnoreConfig fills the same role on ResolvedConfig that SetupCommand does (setupCommands: SetupCommand[]), but twin-servers/index.ts re-exports SetupCommand alongside ResolvedConfig while CopyIgnoreConfig is left out:
export type { TwinServersConfig, TwinServersConfigInput, ResolvedConfig, SetupCommand } from './types';Consumers of the public twin-servers API can currently import ResolvedConfig but have no way to name the type of its .copyIgnore field. Worth adding CopyIgnoreConfig to that export list for parity.
| copyIgnore: { | ||
| ...defaultCopyIgnoreConfig(), | ||
| ...validConfig.copyIgnore, | ||
| }, |
There was a problem hiding this comment.
Minor UX footgun: because folders/files are replaced wholesale rather than merged/appended, a project that only wants to add one extra ignored path (e.g. tmp/traces) and writes copyIgnore: { folders: ['tmp/traces'] } silently loses the audit-results/compare-results/compare-bisect-results defaults — exactly the multi-GB result dirs this PR is meant to keep off the wire. The README example does show repeating the full default list, so this is mitigated by docs, but it's an easy trap since nothing warns when a supplied list omits the defaults. Consider a merge-by-default (with an explicit opt-out) or at least a runtime warning when overriding.
Review summaryWent through the new Overall: the design is sound. The repo-root-relative path normalization ( Two inline notes posted, both minor/non-blocking:
No security or correctness issues found in the matcher/path-normalization logic itself. |
1 participant
Summary
audit-results/,compare-results/, andcompare-bisect-results/on the host by defaultcopyIgnorekeys so configuration typos fail validationHistory
This branch was rebased directly onto current
main; it does not include the deferred demo bisect seed-history work from #161. The granular commits are preserved.Validation
yarn workspace shaka-perf build