Fix failing CI: broken workflow YAML, gutted controllers, RolesGuard DI, stale tests - #249
Merged
chonilius merged 8 commits intoSep 7, 2026
Conversation
.github/workflows/ci.yml's "Run E2E Tests" step had two `run:` keys under
one step — invalid YAML that made the workflow fail to parse at all
("This run likely failed because of a workflow file issue"), so CI never
even reached lint/build/test on any push or PR. The first `run:` (the
one actually taking effect, per YAML's last-key-wins semantics for
duplicate mapping keys) was `npm run test:e2e`; the second, dead one
guarded on DATABASE_URL being set — but DATABASE_URL is hardcoded in this
same step's env block, so that guard could never fire, and the step's own
comment above already explains it was deliberately made unconditional
(MergeFi#164). Removed the dead duplicate, keeping the simple unconditional run.
The app had two separate RolesGuard implementations: the canonical
src/auth/guards/roles.guard.ts (DB-backed, used by bounties/escrow/
github/teams/maintenance-pool controllers), and a second, broken
src/roles.guard.ts that read roles directly off request.user — which,
per JwtStrategy.validate(), only ever contains { userId, username } and
never had a `roles`/`role` field to begin with. That second guard was
also registered globally via APP_GUARD in app.module.ts ("to secure all
role permissions across the entire app"), which is strictly worse than
just broken: global guards run before any per-route @UseGuards(), so
JwtAuthGuard never gets a chance to populate request.user first, meaning
this guard threw "Authentication session not found" on every single
request to every @roles()-decorated endpoint in the app, authenticated
or not.
- Deleted src/roles.guard.ts and its global APP_GUARD registration.
Role-gating is already handled correctly by each controller's own
@UseGuards(JwtAuthGuard, RolesGuard) using the canonical guard.
- Discovered in the process (via a full AppModule e2e bootstrap test):
the canonical RolesGuard needs Repository<User> resolvable in the
*consuming* module's own DI scope, not just transitively through an
imported AuthModule that exports the guard class — re-exporting an
already-constructed provider doesn't re-export that provider's own
constructor dependencies for a class reference resolved via
@UseGuards(). TeamsModule, GithubModule, and BountiesModule (the
modules using @idempotent(), which has the identical requirement for
IdempotencyKey) didn't have these entities in their own
TypeOrmModule.forFeature() call, unlike BountiesModule/EscrowModule
which already did — added them, matching that existing working pattern.
"security: implement multi-tier throttler rate limiting on critical routes" (97f3935) — despite its name — gutted two controllers down to stubs while adding @Throttle to what was left, and CI never ran far enough (see the workflow YAML fix) to catch it: - EscrowController lost fund(), findOne(), and splitRelease() entirely, lost every @idempotent() decorator, lost the toPublicEscrow() response mapping, and release() was reduced to `this.escrowService.release(id, '', '')` — hardcoded empty strings for recipientAddress/recipientId instead of reading them from the request body. - MaintenancePoolController lost create(), list(), findOne(), deposit(), and assignReward() entirely — 89 of 94 lines deleted — leaving one fake `assign-funds` stub ("Falls back safely to your underlying module service method signature") that called nothing real. Both services (EscrowService, MaintenancePoolService) were untouched and remain fully implemented and well-tested; only the controllers wiring requests to them had been deleted. Restored both controllers to their last-known-good shape (recovered via `git show 97f3935^:<path>` / `git show 184957e:<path>`), keeping the @Throttle decorators that were legitimately added afterward on the mutation endpoints. Restoring real @idempotent()/@UseGuards() usage on these controllers surfaced the same DI-scoping issue fixed for RolesGuard in the previous commit, this time for EscrowModule (needed IdempotencyKey) — fixed the same way. Updated the e2e specs exercising these controllers in isolation (they construct their own TestingModule, so they need their own JwtAuthGuard/RolesGuard overrides) and fixed several that used human-readable placeholder IDs ('esc_1', 'bounty_1', 'milestone_1', 'pool_1', 'issue_1') as :id route params — these entities all have real UUID primary keys and every route validates the param with ParseUUIDPipe, so a non-UUID placeholder was always rejected by the pipe before reaching the controller at all.
MilestonesService.resolveIssue() called `escrowService.releasePartial(escrowId, recipientAddress, amount)` — amount and recipientAddress swapped relative to EscrowService.releasePartial's real signature `(escrowId, amount, recipientAddress, recipientId?)`. A Stellar wallet address would have been sent where an amount belongs and vice versa on every milestone payout. The offending line carried a confident-sounding but wrong comment: "FIXED: Aligned argument signature with our 3-arg escrow service update" — the "fix" introduced the bug. Also: MilestonesController's ResolveIssueDto already accepted an optional `recipientId`, but the controller never passed it to the service, and the service didn't even accept a 4th parameter — so it silently went nowhere. Added `recipientId?: string` to resolveIssue()'s signature, wired it through to releasePartial(), and wired dto.recipientId through in the controller. Also fixes the RolesGuard import in milestones.controller.ts (it was pointing at the broken src/roles.guard.ts from the previous commit) and simplifies allocateBudget() to call `this.milestonesService.allocateBudget(id)` directly instead of through an `as any`-cast "does this method exist?" runtime check — it exists. MilestonesModule needed User and IdempotencyKey added to its own TypeOrmModule.forFeature() for the same DI-scoping reason as the previous two commits.
…pecs
- escrow.service.spec.ts: removed a duplicate `let soroban:` declaration
(a syntax error — Jest couldn't even parse the file) and fixed 3
assertions that expected the wrong method name / argument shape for
release() vs releasePartial() (each test asserted both a 'release' and
a 'release_partial' call against what is actually a single soroban.invoke
call — only one could ever be right, and neither matched the omitted
3rd contractOpts argument).
- test/mocks/stellar-sdk.mock.js: the manual mock's
`rpc.Api.GetTransactionStatus` only defined NOT_FOUND, missing SUCCESS
entirely — every "did the transaction succeed" check in
soroban-client.service.spec.ts compared the real 'SUCCESS' string
against `undefined` and always failed.
- github-sync.service.spec.ts: syncIssues()'s tests mocked
`octokit.paginate.iterator`, an API the real implementation has never
called — it fetches one page at a time via `octokit.issues.listForRepo`
and returns `{ saved, nextPage }`, not a flat array (see the method's
own docblock on why: persisting page-by-page instead of collecting a
full paginated result first). Rewrote the 5 affected tests against the
real single-page contract.
- teams.service.spec.ts: expected error text "Team split percentages
must sum to 100" where the actual (and, per the shared validator's
other caller in escrow.service.ts, intentional) label is "team member
split percentages must sum to 100".
118 pre-existing lint errors across ~20 files — never caught because the CI workflow never got past the broken YAML. Split into what's actually worth fixing by hand vs. noise from strict typescript-eslint no-unsafe-* rules applied to inherently loosely-typed Jest mocks: - github.strategy.ts: typed the GitHub OAuth config read off ConfigService (was implicitly `any`) and passport-github2's profile/done callback params (were explicit `any`). - auth.module.ts: replaced a blanket `as any` on the whole JWT module options object with a narrow, documented `as StringValue` cast on just the one field (`expiresIn`) that actually needs it — jsonwebtoken's ms.StringValue template-literal type can't be derived from the configured plain `string` without a runtime format check. - auth.controller.ts: removed `async` from two handoff-exchange methods that never used `await`. - encryption.transformer.ts / github-account.entity.ts: dropped two unused `catch (error)` bindings (4 occurrences). - main.ts: removed unused LoggerService/Logger imports. - escrow.service.ts: typed invokeOnLockedEscrow's `escrow` param as the real `Escrow` entity instead of `any`. - soroban-client.service.ts: typed toScVal()'s return (and therefore invoke()'s scArgs) as the real `xdr.ScVal` instead of an `as any[]` cast. - eslint.config.mjs: added a scoped override disabling no-unsafe-argument/ -assignment/-call/-member-access/-return/-function-type and require-await for **/*.spec.ts and test/**/*.ts. These rules exist to catch real bugs in application code; against Jest mocks (mocked TypeORM repositories, supertest's app.getHttpServer(), jest.fn() return values) they're inherent to how mocking works, not something worth retyping test-by-test.
npm run lint runs eslint --fix with eslint-plugin-prettier wired in as an error-level rule, so fixing the real lint errors elsewhere in this PR also reformatted pre-existing prettier violations in files this PR otherwise doesn't touch (line-wrapping, quote style, one stray indentation typo). No behavior changes — verified via the full test suite before and after.
|
@gideononiru is attempting to deploy a commit to the chonilius' projects Team on Vercel. A member of the Team first needs to authorize it. |
CI's own run of this PR failed at "Run Unit & Integration Tests":
analytics.integration.spec.ts's DataSource uses schema: 'analytics_itest'
with synchronize/dropSchema — but unlike the default `public` schema,
TypeORM doesn't create a named schema that doesn't already exist; it can
only synchronize/drop tables within one that does. GitHub Actions' fresh
postgres service container never has this schema, so
dataSource.initialize() failed with "schema \"analytics_itest\" does not
exist", which the test's own catch-and-warn path treats as "DB not
reachable" and skips — except the test then asserts
`expect(process.env.CI).not.toBe('true')` specifically so a skip in CI
fails loudly instead of silently passing. That assertion is exactly what
tripped here.
This never reproduced locally because a persistent local Postgres
instance keeps schemas across runs, and I hadn't set CI=true (which
gates that strict assertion) when verifying earlier — confirmed by
dropping the schema and re-running with CI=true, which reproduced the
exact CI failure before this fix and passes after.
Fixed by having the test bootstrap the schema itself: a throwaway
DataSource on the default schema runs `CREATE SCHEMA IF NOT EXISTS
analytics_itest` before the real, schema-scoped DataSource initializes.
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
2 participants
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.
Summary
CI on
mainhas been red for a while — the root cause turned out to go much deeper than the workflow file. Fixed everything found while walking the pipeline stage by stage (workflow parse → lint → build → unit/integration tests → coverage → e2e)..github/workflows/ci.yml's "Run E2E Tests" step had tworun:keys under one step (invalid YAML), so the whole workflow failed to parse and never ran anything at all, on any push or PR.src/roles.guard.tsexisted alongside the realsrc/auth/guards/roles.guard.ts, reading roles offrequest.usereven though that object (perJwtStrategy.validate()) only ever contains{ userId, username }. It was also registered globally viaAPP_GUARD, which runs before any per-routeJwtAuthGuard— so it threw "Authentication session not found" on every request to every@Roles()-decorated endpoint in the entire app. Removed it, and along the way found that the realRolesGuard(and@Idempotent()'sIdempotencyInterceptor) need their entity repositories resolvable in each consuming module's own DI scope — addedUser/IdempotencyKeytoTeamsModule,MilestonesModule,GithubModule,BountiesModule,EscrowModule,MaintenancePoolModuleto match the pattern that was already working elsewhere.escrow.controller.tslostfund(),findOne(),splitRelease(), every@Idempotent()decorator, and its response mapping;release()was reduced to hardcoded empty strings instead of reading the request body.maintenance-pool.controller.tslostcreate(),list(),findOne(),deposit(), andassignReward()— 89 of 94 lines gone, replaced by one fake stub. Both services were untouched and fully tested; only the controllers wiring HTTP to them had been deleted. Restored both from their last-known-good state (recovered via git history), keeping the@Throttledecorators added afterward.MilestonesService.resolveIssue()calledescrowService.releasePartial(escrowId, recipientAddress, amount), withamountandrecipientAddressswapped relative to the real signature. Every milestone payout would have sent a wallet address where an amount belongs. Also wiredrecipientIdthrough end-to-end (the DTO accepted it, but the controller and service both silently dropped it).GetTransactionStatusenum missing itsSUCCESSmember,github-synctests mocking an Octokit API the real implementation doesn't call, several e2e specs using non-UUID placeholder IDs against routes that validate withParseUUIDPipe, and a stale error-message assertion.anys, dropped unused imports/catch bindings, removedasyncwith noawait); the remainder —no-unsafe-*violations against Jest mocks — scoped off for test files only, since that's inherent to mocking, not an app bug.Verified locally end-to-end: lint clean, build clean, 288/288 unit+integration tests passing, coverage step clean, 43/43 e2e tests passing.
Test plan
npm run lint— 0 errorsnpm run build— succeedsnpm run test— 288/288 passingnpm run test:cov— cleannpm run test:e2e— 43/43 passing (against a local Postgres)