Conversation
Three preventive measures for the bug class that let main accumulate
a duplicate loadDotEnv / duplicate "private" key / unused helper wiring
across multiple merges without CI noticing:
1. CI typecheck now runs workspace-wide
- Root script `typecheck` expanded to
`tsc --noEmit && pnpm --filter @obsidian-ai-pipeline/core build && pnpm -r typecheck`
- CI step calls `pnpm typecheck` instead of root-only `tsc --noEmit`.
- Catches Duplicate-function-implementation and unused-helper errors
in packages/core and apps/* that the old CI missed.
2. package.json duplicate-key lint
- .github/scripts/check-package-json-duplicates.py uses Python's
json.load(object_pairs_hook=...) to reject duplicate keys (JSON.parse
and default json.load accept them silently, last-wins).
- New CI step runs the linter across all package.json in the repo
(excluding node_modules) before install, fail-fast.
- Emits ::error::file=... annotations for PR UI surfacing.
3. Branch protection docs (docs/branch-protection.md)
- Can't be expressed in repo code; documents the required Web-UI /
gh api settings. The critical flag is
required_status_checks.strict=true (Require branches to be up to
date before merging) which would have forced PR #22 to rebase onto
current main before becoming mergeable, preventing the whole class
of "old base + corrupted main = unresolvable 3-way merge" bug.
Verified locally (simulated CI order):
- package.json lint: 6/6 OK
- pnpm install --frozen-lockfile: clean
- pnpm rebuild better-sqlite3: done
- pnpm test: 120/120 passed
- pnpm typecheck (root + workspace): clean
|
Caution Review failedPull request was closed or merged during review WalkthroughA new CI validation step that checks for duplicate keys in Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Preventive measures for the regression class that enabled PR #22's unresolvable conflicts (see PR #23 post-mortem). Three tightenings of the CI gate + one documentation deliverable.
1. Workspace-wide typecheck in CI
Root
typecheckscript is nowtsc --noEmit && pnpm --filter @obsidian-ai-pipeline/core build && pnpm -r typecheck, and the CITypeScript type checkstep callspnpm typecheckinstead of root-onlypnpm exec tsc --noEmit.Why: the old CI only typechecked root files.
packages/core/src/config/env.tshadloadDotEnvdefined twice andreadPort/safeFileSegment/safeAnalysisFileNamehelpers were never wired to their call sites — all of which TypeScript would have flagged, but-r typecheckwas never run in CI. The duplicate-function-implementation error in core also masked theexactOptionalPropertyTypesviolations inapps/sync/src/sync.ts, compounding the hidden breakage.2.
package.jsonduplicate-key lint (.github/scripts/check-package-json-duplicates.py)New CI step
Lint package.json files for duplicate keys, runs before install for fail-fast behavior. The linter uses Python'sjson.load(object_pairs_hook=...)to reject duplicates at any nesting level —JSON.parseand defaultjson.loadsilently accept them with last-wins semantics.Why: main ended up with
"private": truewritten twice in rootpackage.jsonduring merge history. Neither node nor pnpm complains about that; it only surfaced when I had to hand-merge. Annotations use GitHub's::error file=...::format so PR UI highlights the offending file inline.Tested locally against a synthetic duplicate — linter exits 1 and emits
::error file=package.json::duplicate key 'private'.3. Branch protection docs (
docs/branch-protection.md)Can't be expressed in repo code. Documents the required Web UI /
gh apisettings — criticallyrequired_status_checks.strict=true("Require branches to be up to date before merging"), which would have forced PR #22 to rebase onto current main before becoming mergeable and prevented the whole "old base + corrupted main = unresolvable 3-way merge" scenario.Includes a ready-to-run
gh api PUT /repos/.../branches/main/protectioninvocation for the complete recommended ruleset.Verified locally (simulated CI order)
Also verified the duplicate-key linter correctly fails (exit 1 +
::error::annotation) on a syntheticpackage.jsonwith duplicate"private"keys.Test plan
pnpm typecheckstep passes (was just roottsc)docs/branch-protection.mdsettings on GitHub after mergeSummary by CodeRabbit
Release Notes
Chores
Documentation