Skip to content

fix(ci): repair the verification pipeline — fake-green checks, dead e2e suite, runtime coverage gaps - #563

Merged
yinlianghui merged 5 commits into
mainfrom
claude/verification-pipeline-issues-0c8u5k
Jul 31, 2026
Merged

fix(ci): repair the verification pipeline — fake-green checks, dead e2e suite, runtime coverage gaps#563
yinlianghui merged 5 commits into
mainfrom
claude/verification-pipeline-issues-0c8u5k

Conversation

@yinlianghui

@yinlianghui yinlianghui commented Jul 31, 2026

Copy link
Copy Markdown
Collaborator

Description

Addresses every item in #495. Three layers of this repo's verification reported success without checking anything, and the layer that could have caught the difference never ran.

Everything below was verified by running it, not by reading it.

Note: main moved while this branch was in flight (stage_entry_date, #489). main is merged in and the affected assertions updated — see the last commit. All numbers below are measured on the merged tree.

Type of Change

  • Bug fix (non-breaking change which fixes an issue)
  • CI/CD update
  • Documentation update

Related Issues

Fixes #495

Changes Made

CI checks that passed while checking nothing

  • code-quality.yml — the console.log, TODO/FIXME and large-file checks all scanned packages/, a directory this repo has never had, each with continue-on-error: true. Replaced by scripts/check-source-hygiene.mjs, which scans the real tree (src, test, e2e, scripts), fails on a hit, and fails loudly if a scanned directory ever disappears — so it can never go vacuous again. The tree is clean on all three today, which is what makes the gate enforceable from day one.
  • deploy-docs.yml — copied QUICKSTART.md and PROJECT_SUMMARY.md with no guard; neither has ever existed, so every triggering push failed the workflow. Removed, and docs/ is now copied without || true.
  • pnpm lintcontinue-on-error: true dropped in both workflows. objectstack lint already exits 0 on warnings and suggestions, so the flag suppressed nothing except the errors worth gating on.
  • .eslintrc.json — deleted. No eslint package was installed anywhere to read it, and nothing referenced it. CONTRIBUTING.md:148 told contributors to run pnpm lint "as linting"; it now explains that objectstack lint checks metadata conventions, and points at pnpm typecheck / the hygiene script for code-level checks.
  • .github/labeler.yml — of seven rules, three pointed at paths that have never existed (src/server.ts, src/engine/**, src/triggers/**, src/ui/**, tailwind.config.cjs, src/metadata/**) and ui also named a label the repo does not have, so it could not have applied even with a correct glob. Globs repointed at the real tree; test/labeler-config.test.ts now fails on a glob that matches nothing or a label that does not exist. (Verified against the old config: it flags all eight dead globs plus the missing label.)
  • apps/docs — a Next.js + Fumadocs site with 231 mdx pages, its own package.json and its own lockfile, so root pnpm install/typecheck/build never touched a file of it. New docs-app.yml runs types:check and build on changes. Both verified passing locally.

The e2e suite could not run

  • playwright.config.ts pointed baseURL at 4004 while pnpm dev/start and every doc serve 4001, declared no webServer, and no workflow ran playwright — so the only tests exercising the real kernel had been dead for their whole life.
  • Both specs also treated the data API's 401 as a pass ([200, 401, 403], and a test.skip()), so even on the right port they could only ever prove a route was mounted. Accepting 200 there meant a regression exposing every account anonymously would have kept the suite green.
  • smoke.spec.ts asserted body.data.version === '1.0.0' — that is the ObjectStack health payload schema version, not hotcrm's; no change to this repo can affect it and any platform bump breaks it. Now asserts the shape; hotcrm's own version stays pinned against the manifest in test/smoke.test.ts, where it means something.
  • opportunity-lifecycle.spec.ts had three independent reasons it could never have gone green on real behaviour: the self-skip, reading the record from json.data when the API answers { object, id, record }, and omitting the required crm_account (a 400, not a 401).
  • The suite now boots the server itself (webServer runs build && start, waits on /api/v1/health), authenticates once in e2e/global-setup.ts, and asserts unconditionally — including a dedicated test that the data API is gated. retries: 2 + trace: 'on-first-retry' in CI; e2e.yml runs it.
  • 11 specs pass against a cold database, exercising the real opportunity_lifecycle hook end to end (stage→probability derivation, close-won date stamping, the stage_entry_date stall clock and the days_in_stage formula that reads off it, the closed-record freeze guard rejecting a business-field edit while accepting a narrative one).

Typecheck blind spots

  • tsconfig.json's include was ["src/**/*", "objectstack.config.ts"], so test/, e2e/ and scripts/ — including the 606-line scripts/analytics-reconcile/ — were never typechecked. Widening it surfaced 45 real errors, all fixed here (the reconciler reached for object/categoryField/measures/groupingsDown off types that do not declare them; several tests had unnarrowed undefined).
  • outDir: "./dist" was declared while dist/ holds the objectstack build artifact publish-marketplace.mjs uploads — any tsc run without --noEmit would have scattered .js into what ships. noEmit is now pinned in the config, so the footgun is gone regardless of how tsc is invoked. Module mode moves to preserve/bundler: nothing here is emitted by tsc, and it is what lets extensionless imports and import.meta both resolve.
  • tsx is now a real devDependency behind pnpm reconcile:analytics. The script documented pnpm tsx … as its run command, but tsx was in no package.json and no script invoked it. The documented command now runs (exits 0, 22 surfaces reconciled/skipped).
  • scripts/wow1-live-schema.sh called /api/v1/ai/*, which nothing mounts locally — objectstack.config.ts documents that ai was deliberately dropped from requires. It now preflights and explains that, instead of failing on a bare curl: 404 twenty lines in.

Runtime coverage

before after
Hooks with runtime tests 4 / 24 24 / 24
Flows with runtime tests 3 / 20 17 / 20
Hook-handler statements 23.2% 94.9%
Hook-handler functions 22.4% 95.3%
Hook-handler branches 8.9% 80.4%
Vitest tests 144 359
e2e specs that can run 0 11
  • vitest.config.ts gains coverage config with thresholds set just under the measured numbers, so they lock in this work rather than aspire to it. Coverage is scoped to src/objects/*.hook.ts — the only executable code in src/; including declarative metadata diluted the signal enough to hide the gap in the first place.
  • Flow files are deliberately excluded from line coverage: a flow is an object literal, so importing it scores 100% whether or not any test executes it. test/runtime-coverage.test.ts enforces flow coverage structurally instead, failing when a registered hook or flow has no runtime test, with a PENDING_FLOWS list that may only shrink (a stale entry fails too).
  • Shared harnesses (test/helpers/hook-harness.ts, flow-harness.ts) replace three divergent copies of the in-memory data engine and add the query operators the old ones lacked. This mattered: an equality-only engine silently matches nothing for the $lt/$nin filters every scheduled sweep uses, so those flows could not have been tested against it at all.
  • test/docs-drift.test.ts no longer resolves source files via cwd-relative paths (including the repo-tree guard main added while this was in flight).

⚠️ Defect these tests surfaced

Conditional edges nested inside a loop body never evaluate.

AutomationEngine.registerFlow runs applyConversionsToFlow, which rewrites a bare string condition into a { dialect: 'cel', source } envelope — but only for a flow's top-level edges. It does not recurse into the ADR-0031 control-flow regions (loop.config.body). A condition left as a bare string falls through to the engine's legacy template path, which substitutes {var} templates (there are none) and then string-compares the leftover expression text:

'existingStallTask == null'  →  'existingStallTask' === 'null'  →  false

Verified against @objectstack/service-automation 16.1.0: the same expression passed as an explicit CEL envelope evaluates correctly, which is why every top-level conditional edge in this app works and only the loop-nested ones do not.

Three flows carry conditional edges inside a loop body and are therefore inert past that gate: opportunity_stagnation, contract_renewal, campaign_enrollment. The stalled-deal sweep selects the right deals and then nudges nobody; the renewal sweep never books a renewal.

This is pinned and documented in test/flow-scheduled.test.ts (with a test demonstrating the available fix) rather than fixed here. Fixing it — authoring nested conditions as explicit CEL envelopes, or making the conversion pass recurse upstream — changes what these production sweeps do (they would begin creating tasks, opportunities and notifications), which deserves its own review rather than riding along in a verification-pipeline change. Happy to open that as a follow-up.

Testing

  • Unit tests pass — pnpm test: 359 passed / 20 files (2 skipped, pre-existing)
  • Linting passes — pnpm lint exits 0
  • Build succeeds — pnpm build
  • pnpm verify (now validate + typecheck + lint + hygiene + build + test) exits 0
  • pnpm typecheck clean across src, test, e2e, scripts
  • pnpm test:e2e11 passed against a freshly seeded database
  • apps/docs types:check and build both pass
  • New tests added
  • Verified the new labeler guard actually fails on the old config, and the new hygiene script on a seeded violation

Checklist

  • I have added a changeset (.changeset/verification-pipeline-repair.md)
  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation (CONTRIBUTING)
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective
  • New and existing unit tests pass locally with my changes

Additional Notes

  • package-lock.json was regenerated for the two new devDependencies (tsx, @vitest/coverage-v8); check-stackblitz-lock.mjs passes.
  • test:qa was removed. It ran objectstack test, which scanned qa/*.test.json — a directory that does not exist — against port 3000, and exited 0. Verification pipeline: fake-green CI checks, dead e2e suite, runtime test coverage gaps #495 notes verify omits it; a script that finds nothing and succeeds is the same fake-green pattern this PR is about, so it was deleted rather than wired in.
  • PENDING_FLOWS carries three entries (campaign_enrollment, case_csat_followup, demo_bootstrap), each with a stated reason. The guard stops that list growing silently or going stale, but it is a real remaining gap rather than full coverage.
  • One pre-existing quirk is now visible rather than silent: task_recurrence advances a monthly series with Date.setMonth, so Jan 31 + 1 month lands on Mar 3 instead of clamping to Feb 28. The test asserts current behaviour and flags it for follow-up.

Three layers of this repo's verification reported success without checking
anything, and the layer that could have caught the difference did not run.

CI checks that checked nothing:
- code-quality.yml's console.log / TODO / large-file checks all scanned
  `packages/`, a directory this repo has never had, each with
  `continue-on-error: true`. Replaced by scripts/check-source-hygiene.mjs,
  which scans src/test/e2e/scripts, fails on a hit, and fails loudly if a
  scanned directory disappears.
- deploy-docs.yml copied QUICKSTART.md and PROJECT_SUMMARY.md unguarded;
  neither exists, so every triggering push failed the workflow.
- `continue-on-error` removed from `pnpm lint` in both workflows —
  `objectstack lint` already exits 0 on warnings, so the flag hid only real
  errors.
- .eslintrc.json removed (no eslint installed anywhere to read it) and
  CONTRIBUTING corrected to describe what `pnpm lint` actually does.
- labeler.yml: three of seven rules pointed at paths that never existed and
  `ui` at a label the repo does not have. Globs repointed; a new test fails
  on a glob that matches nothing or a label that does not exist.
- apps/docs (Next.js, 231 mdx pages, own lockfile) is now typechecked and
  built by docs-app.yml. Nothing compiled it before.

The e2e suite could not run:
- playwright.config.ts pointed baseURL at 4004 while the server serves 4001,
  declared no webServer, and no workflow ran it.
- Both specs treated the data API's 401 as a pass, so even on the right port
  they proved only that a route was mounted. smoke.spec.ts also pinned a
  platform-owned health constant, and opportunity-lifecycle.spec.ts read the
  record from the wrong envelope key and omitted a required field.
- The suite now boots the server, authenticates once via globalSetup, and
  asserts unconditionally — including that the data API IS gated. retries and
  trace are configured for CI; e2e.yml runs it.

Typecheck blind spots:
- tsconfig covered only src/, leaving test/, e2e/ and scripts/ unchecked.
  Widening `include` surfaced 45 real errors, all fixed here.
- module mode moves to preserve/bundler (nothing here is emitted by tsc) and
  noEmit is pinned so no stray tsc run can scatter output into the dist/ that
  publish-marketplace.mjs uploads.
- tsx is a real dependency behind `pnpm reconcile:analytics`; the command the
  analytics-reconcile script documented could not run.
- wow1-live-schema.sh preflights the `ai` capability and explains why a local
  server does not mount it.

Runtime coverage:
- Hooks 4/24 -> 24/24 tested; flows 3/20 -> 17/20, including all six
  scheduled sweeps.
- Hook-handler coverage: 23% -> 95% statements, 22% -> 95% functions, with
  vitest thresholds set just under the measured numbers.
- Shared hook/flow harnesses replace three divergent copies of the in-memory
  data engine and add the query operators the old ones lacked; an
  equality-only engine silently matches nothing for the $lt/$nin filters
  every scheduled sweep uses.
- test/runtime-coverage.test.ts fails when a hook or flow arrives untested.
- docs-drift.test.ts no longer resolves flow files relative to cwd.

Defect these tests surfaced: conditional edges nested inside a `loop` body
never evaluate — the flow conversion pass wraps bare string conditions into
CEL envelopes only for top-level edges, so a loop-nested condition falls
through to the engine's legacy path and is string-compared. That leaves
opportunity_stagnation, contract_renewal and campaign_enrollment inert past
their gates. Pinned and documented in test/flow-scheduled.test.ts rather than
fixed here: the fix changes what those production sweeps do and belongs in
its own change.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0127MccuEdXF7pxJFmaiD7UU
@vercel

vercel Bot commented Jul 31, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
Project Deployment Actions Updated (UTC)
hotcrm Ignored Ignored Jul 31, 2026 4:26am

Request Review

@github-actions github-actions Bot added documentation Improvements or additions to documentation ci/cd dependencies configuration labels Jul 31, 2026
Comment thread test/labeler-config.test.ts Fixed
claude added 3 commits July 31, 2026 03:12
CodeQL flagged it on #563. The file walks directories with readdirSync's
withFileTypes and never stats anything — the import was left over from an
earlier size-check draft that moved into scripts/check-source-hygiene.mjs.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0127MccuEdXF7pxJFmaiD7UU
main moved `days_in_stage` from a stored column to a formula over a new
`stage_entry_date` (42a1d33, #489). PR checks run against the merge commit,
so the branch went red on assertions that were correct when written:

- opportunity_lifecycle now re-stamps `stage_entry_date` on a stage change
  instead of zeroing `days_in_stage`. The hook tests assert the stored column,
  and gain two cases the change makes load-bearing: the clock starts on insert
  (a deal that never moves must still be visible to the sweep), and an edit
  that does not change the stage must not reset it.
- opportunity_stagnation filters `stage_entry_date < {TODAY() - 14}`, so the
  sweep tests seed dates rather than day counts. The boundary case now pins
  `$lt` (exactly-14 must not fire) and a null-clock row is added, since a null
  does not satisfy `$lt` and must be skipped.
- The e2e lifecycle spec asserted `days_in_stage` off the PATCH response.
  Formulas are evaluated over query results and are not echoed on a write, so
  it re-reads the record and asserts the derived value there — which proves the
  formula resolves end to end rather than just that the hook wrote a date.

Also anchors the repo-tree drift guard main added on REPO_ROOT. It read
`join(docFile)` and `join('src', dir)` relative to cwd, the same defect this
branch fixed in the rest of the file: correct from the repo root, ENOENT from
anywhere else.

Full gate green on the merge: 359 tests, 11 e2e specs against a cold database,
hook coverage 94.9% statements / 95.3% functions.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0127MccuEdXF7pxJFmaiD7UU
@yinlianghui
yinlianghui merged commit 0ee9057 into main Jul 31, 2026
11 checks passed
yinlianghui pushed a commit that referenced this pull request Jul 31, 2026
The push that merged main after #563 landed (efb0707) delivered a
`synchronize` event — `Label Pull Request` and the Vercel check both ran on it
— but none of the `pull_request`-triggered workflows started, so the head
commit carried no CI result and the PR could not be evaluated. Empty commit to
re-run them; no content change.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0127MccuEdXF7pxJFmaiD7UU
yinlianghui pushed a commit that referenced this pull request Jul 31, 2026
#565 landed the same CEL-envelope fix this branch carried, using the `P`
tagged-template helper from @objectstack/spec — a cleaner notation, so the flow
files take main's version wholesale. What remains here is the part #565 did not
cover, plus the two red tests its merge left on main.

main is currently RED. Both failures are tests this branch's parent (#563)
introduced, and both fired exactly as designed:

  - flow-scheduled: the KNOWN DEFECT case asserted the stagnation gate stays
    shut. #565 opened it, so the test failed with the message it was written to
    emit — "gate now opens — rewrite this test". Rewritten here to assert the
    real behaviour (nudge created, idempotent across sweeps, re-arms once the
    task completes).
  - flow-record-change: `opportunity_approval` asserted `typeof
    startCondition === 'string'`. #565 converted it to an envelope. The
    assertion was pinning the NOTATION rather than the thing that matters, so
    it now accepts either form and checks a non-empty expression exists.

Second defect #565 left reachable: `timestamp(currentContract.end_date)` throws
"timestamp() requires a string in ISO 8601 format" because `end_date` is a DATE
field arriving as YYYY-MM-DD. While the condition was a bare string it was
never evaluated, so this was latent; wrapping it as a real envelope makes it
throw mid-sweep. Appending T00:00:00Z fixes it — verified load-bearing by
reverting it, which fails 4 contract_renewal tests.

So on main as it stands, contract_renewal books nothing: the gate evaluates and
then blows up. That is a live regression, not a theoretical one.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0127MccuEdXF7pxJFmaiD7UU
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ci/cd configuration dependencies documentation Improvements or additions to documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Verification pipeline: fake-green CI checks, dead e2e suite, runtime test coverage gaps

3 participants