fix: retrospective review fixes for merged PR #25#110
Merged
Conversation
…po matcher Retrospective review of merged PR #25 (v0.7.2 "ECC compat + dogfooding bridge"), which wired `bough hook handle`'s quality-gate dispatch. Two bugs in that wiring are still live at HEAD (dispatchQualityGates and buildMatchContext are unchanged since v0.7.2, having survived the v0.9 architecture reset): 1. dispatchQualityGates resolved cfgPath as a bare cwd-relative ".bough.yaml" (or $BOUGH_CONFIG), instead of going through resolveMonorepoRoot + resolveConfigPath like every sibling dispatcher in this file (see resolveObserverConfig, fixed for the same class of bug in 487ea4c). Per this file's own documented ECC model, every sub-repo / worktree session pools into one monorepo project, so a `bough hook handle` invocation with cwd inside e.g. <root>/extremo-api/ (the ordinary case) silently never found <root>/.bough.yaml and no quality gate ever ran outside the exact root directory. 2. buildMatchContext never populated qualitygate.MatchContext.Repo, so a gate's on_repo matcher (config.QualityGateCfg.OnRepo, documented and schema-validated since v0.7.1) always compared against the empty string and could never match — a dead feature from day one. Fixes both: dispatchQualityGates now resolves root via resolveMonorepoRoot and the config via resolveConfigPath (keeping $BOUGH_CONFIG as the highest-priority override), and a new repoNameFromCwd helper derives the sub-repo name from cwd (handling both the worktrees/ and legacy .worktrees/ layouts) to populate MatchContext.Repo. Added table-driven tests for repoNameFromCwd plus regression tests that reproduce both bugs end-to-end (gate now runs from a sub-repo cwd; on_repo correctly includes/excludes based on the derived repo).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Retrospective
/code-reviewsweep of merged PR #25 that shipped without pre-merge review.PR #25 ("v0.7.2: ECC compat + dogfooding bridge") wired
bough hook handle's quality-gate dispatch (dispatchQualityGates/buildMatchContextininternal/cli/hook.go). That code is unchanged since v0.7.2 — it survived the later v0.9 architecture reset (which deletedinternal/ecc/and rewrote the ECC import path elsewhere) — and has two still-live bugs:dispatchQualityGatesresolved its config path as a bare cwd-relative.bough.yaml(or$BOUGH_CONFIG), instead of going throughresolveMonorepoRoot+resolveConfigPathlike every sibling dispatcher in the same file (resolveObserverConfig,dispatchEvolveClaudeMD). This file's own docs describe the intended model: every sub-repo / worktree session pools into one monorepo project, sobough hook handleordinarily runs with cwd inside e.g.<root>/extremo-api/, not at<root>itself. From there, the old code's bareos.Stat(".bough.yaml")always missed the canonical config, so no quality gate ever ran except when the hook happened to fire with cwd exactly at the monorepo root. This is the same class of bug already fixed fordispatchObserverAutostart(commit487ea4c) but never applied to this sibling.on_repomatcher was dead code from day one.buildMatchContextnever populatedqualitygate.MatchContext.Repo, so a gate declaringon_repo:(a documented, schema-validatedQualityGateCfgfield) always compared against the empty string and could never match — silently disabling any gate an operator scoped to a specific sub-repo.Fix:
dispatchQualityGatesnow resolves the monorepo root and goes throughresolveConfigPath(keeping$BOUGH_CONFIGas the highest-priority override, so that existing behavior is preserved), and a newrepoNameFromCwdhelper derives the sub-repo name from cwd — handling both theworktrees/and legacy.worktrees/layouts — to populateMatchContext.Repo.Test plan
go build ./...go test ./...golangci-lint run ./...TestDispatchQualityGates_ResolvesConfigFromSubRepoCwd,TestDispatchQualityGates_OnRepoMatchesDerivedRepo,TestRepoNameFromCwd,TestBuildMatchContext_CarriesRepo