Add advisory business contract test lane - #357
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (3)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 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 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| { | ||
| plugins: [react()], | ||
| resolve: { | ||
| alias: { | ||
| '@': path.resolve(__dirname, './src'), | ||
| 'server-only': path.resolve(__dirname, './src/test/stubs/server-only.ts'), | ||
| }, | ||
| }, | ||
| test: { | ||
| ...sharedTestConfig, | ||
| name: 'business', | ||
| include: ['src/**/*.contract.test.{ts,tsx}'], | ||
| exclude: ['node_modules', '.next'], | ||
| testTimeout: 30_000, | ||
| }, | ||
| }, |
There was a problem hiding this comment.
extends: true — won't auto-inherit root alias changes
The unit, integration, and rls projects all specify extends: true and rely on the root-level plugins and resolve.alias config. The business project instead duplicates those entries manually. If a new alias is added at the root level (e.g., a future stub for a Next.js internal), the business project won't pick it up automatically, causing Module not found failures that appear only in the advisory contract lane. Using extends: true and omitting the manually duplicated plugins and resolve block would keep the business project consistent with the other three projects and avoid this silent maintenance trap.
|
Kept open through the 2026-07-03 PR triage — the contract-test lane is worth landing, but the branch now conflicts with current main in .circleci/config.yml + CODEOWNERS. Needs a rebase onto main, then it can ride the next merge train. |
1 participant
Summary
Verification
Greptile Summary
This PR introduces an advisory Vitest
businessproject that runs*.contract.test.tsfiles covering GolfHelm stats formulas, CoachHelm metric direction/evidence/budget, product-trust UI states, access/RLS boundaries, and import normalization — wired into CI as a non-blockingcontinue-on-errorjob that is explicitly excluded from the hardallgate.business-contractsjob usescontinue-on-error: true, is absent from thealljob'sneedsarray, and uses the same action versions as every other job inci.yml.starterormissing, four openDECISION-*items are tracked, and the discovery document clearly separates safe tests added now from tests needing adapters or product decisions.businessvitest project omitsextends: true(requiring manual alias duplication that won't auto-inherit future root changes), and the source-sniffing approach instates.contract.test.tsandgolf-access.contract.test.tswill generate false advisory failures on harmless string-literal refactors.Confidence Score: 4/5
Safe to merge — the advisory lane is correctly wired and cannot block merges.
The CI integration, matrix documentation, and CoachHelm/stats contract logic are all sound. The two structural concerns — the vitest project skipping
extends: trueand the source-sniffing approach in the states/access contracts — are real maintenance risks but don't affect production behavior. The mislabeled "null-honest" assertion onnormalizeToYards(null)documents an existing coercion that may mislead future maintainers about what null-honesty guarantees for normalization utilities.vitest.config.ts (business project config), src/contracts/imports/normalization.contract.test.ts (null-honesty label), src/contracts/product-trust/states.contract.test.ts and src/contracts/access/golf-access.contract.test.ts (source-sniffing fragility).
Important Files Changed
business-contractsjob withcontinue-on-error: true; correctly omitted from theallhard-gateneedsarray and uses the same action versions as every other job in the file.businessproject with its ownpluginsandresolveblock instead ofextends: true; future root-level alias additions won't be inherited automatically, unlike the other three projects.'putt', not'putting') which assumes the stat calculator normalizes internally.METRIC_RENDER_CONFIG, improvement sign polarity, citation verification, sample damping, bounds checks, and budget metadata coverage.normalizeToYards(null, 'feet')returns0, documenting null-to-zero coercion that contradicts the null-honesty contract definition.readFileSync+toContainto detect product-trust state invariants; will break on harmless string-literal refactors without real regressions — acceptable asstarterstatus but carries a maintenance burden when promoted.states.contract.test.ts; correctly verifiesgolf_team_coach_staffusage and cross-tenant RLS test presence but will false-positive on code reformatting.test:businessandverify:businessscripts;verify:businessis a thin alias fortest:businesswith no additional checks (typecheck, lint), consistent with the advisory-only intent.Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD PR[PR Push] --> CI[CI Jobs] CI --> HARD[Hard Gate Jobs] CI --> ADV[Advisory Jobs] HARD --> typecheck[TypeScript] HARD --> lint[Lint / Lint Ratchet] HARD --> unit[Unit Tests] HARD --> build[Build] HARD --> rls[Supabase RLS] HARD --> dbTypes[DB Types Drift] HARD --> schema[Schema Invariants] HARD --> knowledge[Feature Knowledge] HARD --> ALL{all gate} ALL -->|required checks pass| MERGE[✓ Merge allowed] ALL -->|any fail| BLOCK[✗ Blocked] ADV --> BC[business-contracts\ncontinue-on-error: true] BC --> BT[npm run verify:business\nvitest run --project business] BT --> SC1[golf/stats.contract.test.ts\nGH-STATS-001…009] BT --> SC2[coachhelm/truth.contract.test.ts\nCH-AI-001…009] BT --> SC3[product-trust/states.contract.test.ts\nTRUST-001,004,005] BT --> SC4[access/golf-access.contract.test.ts\nCH-AI-010] BT --> SC5[imports/normalization.contract.test.ts\nTRUST-003] BC -->|fail| SIGNAL[Advisory signal only\nNo merge block] BC -->|pass| INFO[Radar green]%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%% flowchart TD PR[PR Push] --> CI[CI Jobs] CI --> HARD[Hard Gate Jobs] CI --> ADV[Advisory Jobs] HARD --> typecheck[TypeScript] HARD --> lint[Lint / Lint Ratchet] HARD --> unit[Unit Tests] HARD --> build[Build] HARD --> rls[Supabase RLS] HARD --> dbTypes[DB Types Drift] HARD --> schema[Schema Invariants] HARD --> knowledge[Feature Knowledge] HARD --> ALL{all gate} ALL -->|required checks pass| MERGE[✓ Merge allowed] ALL -->|any fail| BLOCK[✗ Blocked] ADV --> BC[business-contracts\ncontinue-on-error: true] BC --> BT[npm run verify:business\nvitest run --project business] BT --> SC1[golf/stats.contract.test.ts\nGH-STATS-001…009] BT --> SC2[coachhelm/truth.contract.test.ts\nCH-AI-001…009] BT --> SC3[product-trust/states.contract.test.ts\nTRUST-001,004,005] BT --> SC4[access/golf-access.contract.test.ts\nCH-AI-010] BT --> SC5[imports/normalization.contract.test.ts\nTRUST-003] BC -->|fail| SIGNAL[Advisory signal only\nNo merge block] BC -->|pass| INFO[Radar green]Comments Outside Diff (2)
src/contracts/imports/normalization.contract.test.ts, line 688-689 (link)The test is named "null-honest" but then asserts
normalizeToYards(null, 'feet')returns0. Returning0for a null input is the opposite of null-honesty as defined by GH-STATS-006 — it silently converts missing data to a zero value. Any downstream function that receives this0as adistance_to_hole_beforevalue might produce a non-null SG result instead of propagating the missing-data signal. The assertion documents an existing behavior that should be explicitly noted as an intentional internal conversion (for mathematical purposes) rather than a general null-honesty guarantee, or the test name should be corrected so future readers aren't misled into treatingnormalizeToYards(null)as compliant with the null-honesty contract.src/contracts/product-trust/states.contract.test.ts, line 727-758 (link)readFileSync+toContainis brittle for contract testsAll three tests in this file (and similarly in
src/contracts/access/golf-access.contract.test.ts) detect product-trust invariants by reading raw source files and asserting that specific string literals exist. An advisory lane failure from this approach can be triggered by an entirely safe refactor — e.g., renaming "Something Went Wrong" to "An Error Occurred" or reformatting thesetStatsErrorcall — even when the underlying state separation is preserved. This creates noisy advisory failures that aren't genuine product-trust regressions, which is exactly the kind of radar noise the issue drafts warn about. The approach is noted asstarterstatus in the matrix, so this isn't a blocker, but the maintenance cost should be tracked when these contracts are promoted towardcovered.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Reviews (1): Last reviewed commit: "Add advisory business contract test lane" | Re-trigger Greptile