Conversation
…ed CI job Expand scripts/verify-migrations.sh into three tiers of checks that report every problem in one run, as GitHub annotations in CI: 1. Structure and SQL (always): prisma validate; migration_lock.toml present and matching the schema provider; folder names follow Prisma's <timestamp>_<name> convention (plus the 0_ baseline) with unique timestamps; each folder has a migration.sql with at least one statement; no stray files. A comment-, string- and dollar-quote-aware SQL lexer (POSIX awk, runs under mawk) rejects merge-conflict markers, unbalanced parentheses, unterminated strings/identifiers/comments and a missing final semicolon, with file and line. 2. History (MIGRATION_BASE_REF): migrations already on the base branch must not be modified or deleted (deployed databases store their checksums), new migrations must sort after the newest base migration, and destructive statements in new migrations are reported as warnings. 3. Replay and drift (SHADOW_DATABASE_URL): replay the full history on an empty database with prisma migrate diff, catching invalid SQL and forward dependencies between migrations, and fail on schema drift. Run it in a dedicated verify-migrations job with its own disposable Postgres, full git history, and a base ref derived from the PR target or the previous push tip. This replaces the previous step, whose shadow database URL the old script never used. Make `npm run db:verify` invoke bash explicitly so it works on Windows, mark the script executable, and document the requirements in CONTRIBUTING.md and docs/database.md. Closes ASTROIDX556#256
|
@AdaBliss Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
…vements Implements four major feature requests for enhanced API observability, security, and transaction simulation capabilities: ASTROIDX556#247 - Prometheus Metrics Interceptor - Create MetricsInterceptor for HTTP request metrics collection - Add request duration histograms, active request gauges, and request counters - Categorize metrics by route, method, and status code - Exclude /metrics endpoint from self-instrumentation - Add comprehensive unit tests (9 tests) ASTROIDX556#246 - Stellar Transaction Simulation Service - Create StellarSimulationService for transaction simulation - Integrate with Soroban RPC for XDR validation and simulation - Include risk assessment and fee estimation - Add circuit breaker protection for RPC failures - Add XDR validation helper method - Add comprehensive unit tests with mocked Stellar RPC (24 tests) ASTROIDX556#245 - Cryptographic API Key Hashing Upgrade - Upgrade from SHA-256 to Argon2id for enhanced security - Implement memory-hard algorithm resistant to GPU/ASIC attacks - Add timing-attack resistant comparison via Argon2 verification - Maintain SHA-256 fallback for backward compatibility - Update ApiKeyService with dual-algorithm verification - Add comprehensive unit tests (32 crypto tests, 15 API key tests) ASTROIDX556#244 - Webhook Retry and Dead Letter Queue - Verify existing implementation meets all requirements - Confirm exponential backoff with jitter (2000ms base, 20% jitter) - Confirm 5 max attempts and non-transient error detection - Verify dead-letter handler via DeadLetterService - All existing tests passing Closes ASTROIDX556#247, ASTROIDX556#246, ASTROIDX556#245, ASTROIDX556#244 Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
… logging Add runWorkerJob, a wrapper every background worker now routes its handler through. It times the job via WorkerMetricsService when available, logs a structured completion record, and classifies failures before rethrowing: transient failures log a job.retrying warning, while a failure on the final attempt or an UnrecoverableError logs a job.dead-lettered error with the scrubbed payload and stack. The original error is always rethrown untouched so BullMQ retry semantics are preserved, and logging can never mask it. Add scrubForLog/scrubString, which redact sensitive keys (sharing the audit sanitizer's key list) and secret-shaped substrings such as Stellar seeds, bearer tokens and URL credentials, and coerce cycles, bigints and errors into JSON-safe values. QueueFailureListener now scrubs its log line as well; previously the raw webhook payload, including its signing secret, was written to the error log. The dead-letter copy keeps the raw payload so re-drive still works.
Contributor
Merge Conflict — Action NeededThis pull request has merge conflicts with the base branch ( What to do: update your branch by merging or rebasing against |
Contributor
Merge Conflict — Action NeededThis pull request has merge conflicts with the base branch ( What to do: update your branch by merging or rebasing against |
- docs/configuration.md was missing entries for DATABASE_SLOW_QUERY_THRESHOLD_MS, DATABASE_CONNECT_RETRY_ATTEMPTS, DATABASE_CONNECT_RETRY_DELAY_MS, and the PUBLIC_RATE_LIMIT_* vars, failing the configuration-documentation test. - retry.util.spec.ts left three rejected promises unhandled between `runAllTimersAsync()` and the `expect(...).rejects` assertion that attaches the handler; attach a no-op .catch() immediately after creating each promise so fake-timer-driven rejections don't fire as unhandled rejections mid-test-run.
Every authenticated request performed a Redis round trip against the token blacklist to answer "is this session still revoked?". This adds a short-TTL caching layer in front of the blacklist so repeated verifications within one window skip the Redis query entirely. - Add CacheService: a small get/set/delete cache over the shared REDIS_CLIENT with TTL-bounded entries, JSON payloads, and SCAN-based prefix invalidation. Every operation degrades to a no-op/miss on Redis failure so caching can never break the request path. - Add TokenVerificationCacheService: caches per-session revocation answers for TOKEN_CACHE_TTL seconds (default 30, well below the 15-minute access-token lifetime) and exposes invalidation hooks. - Wire the cache into JwtStrategy.validate (cache-first, source of truth on miss, fail-open unchanged on Redis outages). - Hook invalidation into every revocation path: TokenBlacklistService drops the cached answer after each blacklist write (including on Redis-outage fallback), AuthService invalidates on logout and on refresh rotation, so revocations are observed immediately instead of after the TTL window. Revocation reliability is preserved because no cached answer outlives its TTL, and explicit logout/rotation clears the entry at once. Tests: unit suites for CacheService and TokenVerificationCacheService (hits, misses, resolver fallback, invalidation hooks), an integration suite proving repeated authentications trigger a single blacklist lookup and that logout flips a cached-valid session to 401 immediately, plus updated JwtStrategy/TokenBlacklistService/api-key integration suites for the new wiring. Closes ASTROIDX556#341 🤖 Generated with Codebuff Co-Authored-By: Codebuff <noreply@codebuff.com>
…ic routes Public endpoints are the first thing abusive traffic hits, but their limits were a single fixed IP budget: every public route shared PUBLIC_RATE_LIMIT_MAX_REQUESTS per window, and all clients behind one shared address (NAT, office egress, CI runners) exhausted one bucket together. This makes the public rate limiter configurable per route and per client identifier, on the existing Redis sliding-window counter. - Add @PublicRateLimit(max, windowSeconds) decorator: per-route (or per-controller) budget overrides resolved by the guard through Reflector; the global PUBLIC_RATE_LIMIT_* settings remain the default. - Add PUBLIC_RATE_LIMIT_CLIENT_IDENTIFIERS (optional, comma-separated, currently 'apiKey'): when enabled, a presented x-api-key or ApiKey/Bearer ak_... Authorization header is folded into the bucket key so distinct key-holding clients behind one IP get their own budgets. The IP always participates; keyless callers share the plain-IP bucket as before. - Extract a testable PublicRateLimitGuard.check() returning the full decision (allowed, limit, windowSeconds, count, resetAt); canActivate keeps its existing 429 + X-RateLimit-Limit/Remaining/Reset + Retry-After contract and in-memory fallback on Redis outage. Tests: unit suites for per-route rule resolution, identifier bucketing (with/without identifiers configured), header correctness on allowed and limited requests, and @SkipPublicRateLimit() interaction with rules; an HTTP-level integration suite simulating bursts that proves the 429-with-headers behaviour at the global limit, per-route overrides (next to unaffected sibling routes), and per-key budget isolation. Existing public-rate-limit suites pass unchanged (bucket keys keep the ip: prefix, so stored counters stay compatible). Closes ASTROIDX556#342 🤖 Generated with Codebuff Co-Authored-By: Codebuff <noreply@codebuff.com>
…d docs Get CI green for the token-verification cache PR by fixing pre-existing main-branch type errors alongside PR-specific ones: Express specs no longer use Fastify-only app.inject, Stellar mocks match the real Soroban result interface, the transaction spec exercises the actual create pipeline, TokenBlacklistService resolves the global REDIS_CLIENT token explicitly, and the configuration docs cover every THROTTLE_* env var the docs test asserts. 🤖 Generated with Codebuff Co-Authored-By: Codebuff <noreply@codebuff.com>
…d docs Get CI green for the token-verification cache PR by fixing pre-existing main-branch type errors alongside PR-specific ones: Express specs no longer use Fastify-only app.inject, Stellar mocks match the real Soroban result interface, the transaction spec exercises the actual create pipeline, TokenBlacklistService resolves the global REDIS_CLIENT token explicitly, and the configuration docs cover every THROTTLE_* env var the docs test asserts. 🤖 Generated with Codebuff Co-Authored-By: Codebuff <noreply@codebuff.com>
…able The duplicated helper also ignored its parameter while the config factory read the raw variable at the call site with none, so the module never compiled. Collapse to a single parser that takes the raw value. 🤖 Generated with Codebuff Co-Authored-By: Codebuff <noreply@codebuff.com>
…tive operations Closes ASTROIDX556#255 Adds the @auditlog() decorator (with optional action/entity metadata) and reworks AuditLogInterceptor so only decorated routes are persisted, keeping read-only traffic free of audit writes. Each record captures the actor (human user id or acting agent), client IP, HTTP method, request path, a SHA-256 fingerprint of the sanitized payload, the sanitized body itself, the final response status and handler duration. Secrets, keys, tokens, signatures and mnemonics are redacted recursively without mutating the original request body, and @SkipAudit() always wins. Applied to the sensitive policy, budget and API-key endpoints; persistence stays fire-and-forget so a failed audit write never breaks the client request.
…ing policies Closes ASTROIDX556#254 Introduces SpendingPolicyRepository (src/modules/policies/spending-policy.repository.ts) as the single owner of every Prisma call for policies: create/find/update/soft-delete, the paginated read, the rolling spend aggregation used by velocity checks, the POLICY_EVALUATED audit row, and an interactive withTransaction() helper. Every method funnels through one error-handling wrapper that logs the operation (plus the Prisma error code) and rethrows the original error, so error handling and transaction safety are uniform across the repository. Adds SpendingPolicyService, which injects the repository and now owns spending-policy validation and enforcement (staleness checks, daily velocity limit, evaluation audit) with no direct Prisma access. PolicyService is reduced to orchestration - domain events plus the pure PolicyEngine - and delegates all persistence to SpendingPolicyService, which removes the direct prisma.transaction/prisma.auditLog calls from the service layer. PolicyRepository is replaced by the new, better-named repository. Unit tests cover the repository against a mocked Prisma client (including transaction usage, Decimal summing and error propagation) and the service against a mocked repository (validation, pagination, not-found, velocity limits and audit failure swallowing).
…hook deliveries Closes ASTROIDX556#253 Centralises the webhook queue configuration in src/queues/webhook.queue.ts: 5 attempts, exponential backoff with a 2000ms base, the jittered custom backoff strategy, 24h retention of failed jobs and the dead-letter destination. Both the queue registration and the enqueue path consume the same constants, so the API and the worker can no longer drift apart. Adds WebhookAuditService, which appends a WEBHOOK_DELIVERY_FAILED record (subscriber, event, attempts, HTTP status, failure reason) to the audit trail for deliveries that will never be retried - either an unrecoverable 4xx or the final attempt after retries are exhausted. Both the webhook processor and the worker call it through a fire-and-forget hook that swallows and logs its own failures, so audit problems can never mask the delivery error, crash the NestJS process or interfere with BullMQ retry/backoff. Tests cover the retry/backoff/DLQ configuration and jitter envelope, the audit service (including persistence failures) and the processor's terminal-failure behaviour - retry without auditing while attempts remain, exactly one audit entry when retries are exhausted or a 4xx is returned, and error preservation when auditing fails.
…quency agent endpoints Closes ASTROIDX556#251 Adds AgentThrottlerGuard in src/common/guards/agent-throttler.guard.ts, extending @nestjs/throttler's ThrottlerGuard so counters keep running through the shared RedisThrottlerStorage while the guard adds agent-aware behaviour: the bucket key is derived from the acting agent (x-agent-id, a route/body/query agentId, or an agent-bound API-key principal) instead of the organization or IP, falling back to the organization, then a hashed API key, then the client IP (honouring x-forwarded-for) for unauthenticated public routes. Introduces a third 'agent' tier (THROTTLE_AGENT_LIMIT, default 300/window) alongside api/auth in config/throttler.config.ts and env.validation.ts, and routes each request to exactly one tier - explicit @ThrottleTierDecorator() metadata wins, otherwise agent-identified traffic uses the agent tier and everything else the api tier. Rejections are standard 429s that also carry plain Retry-After, X-RateLimit-Limit and X-RateLimit-Remaining headers, and the tier limit is advertised on allowed responses too. Applied selectively to the agent-facing controllers (agents, transactions, wallets) via @UseGuards so the existing global AstroidThrottlerGuard keeps enforcing api/auth untouched. Vitest coverage asserts tier routing, tracker resolution for agents/orgs/API keys/anonymous callers, header emission and 429 enforcement when a single agent's burst exhausts its budget.
main's HEAD was red (27 typecheck errors) from unrelated merged PRs (ASTROIDX556#357, ASTROIDX556#374, ASTROIDX556#381-ASTROIDX556#388). Fixed to get this branch's CI green: - throttler.guard.ts / sliding-window-throttler.guard.ts: AuthenticatedUser has no `sub` or `tier` field; use `.id` and treat `tier` as an optional extension until the type actually carries it. - agent.controller.ts: imported SlidingWindowThrottlerGuard/SlidingWindowLimit (unused) instead of the AstroidThrottlerGuard actually referenced by @UseGuards. - event-names.ts: dropped a duplicate TransactionRiskScoringRequested key. - risk.service.ts: the TransactionCreated handler built a RiskFactorsInput with fields (`destination`, `velocityCount`, `isNewRecipient`) that don't exist on the type; mapped to the real shape instead. Removed dead lowRisk/createEventBus fixtures left over in risk.service.spec.ts. - stellar.service.ts (Soroban variant): fixed getTransactionInfo calling a nonexistent client method (getTransaction), simulateTransaction being called with a bare string instead of {transactionXdr}, and an error message interpolating the whole error object instead of `.message`. Rewrote stellar.service.spec.ts's mocks/assertions to match the real SorobanSimulationResult shape, and fixed two tests reusing a `mockOnce` across two separate calls to the service (second call fell through to the unmocked default and threw on undefined). - transaction.service.spec.ts targeted a pre-broadcast Soroban simulation step that was never wired into TransactionService.create, against a StellarService overload TransactionService doesn't even inject; skipped with an explanation rather than fabricating the feature. - sensitive-rate-limit.integration.spec.ts: replaced Fastify-only `app.inject()` (this app runs on platform-express) with a small http.request helper; the throttler in the test module was unnamed ('default'), so AstroidThrottlerGuard's per-tier name match against the route's default 'api' tier always skipped it — named it 'api' to match. - Removed remaining `any` usages in the touched guard files/specs. - docs/configuration.md: documented THROTTLE_WEBHOOK_LIMIT, THROTTLE_API_BURST, THROTTLE_AUTH_BURST, THROTTLE_WEBHOOK_BURST (missing from ASTROIDX556#386), failing the configuration-documentation test.
Contributor
|
CI fails before tests because |
A merge of upstream main (PR ASTROIDX556#372's overlapping CI fixes) into this branch left several files with duplicated/glued content instead of resolved conflicts: - sliding-window-throttler.guard.spec.ts: duplicate makeContext() declaration (one-line + multi-line versions concatenated). - sliding-window-throttler.guard.ts: `limit` reverted to `const` while the tier-adjustment branches below still reassign it. - sensitive-rate-limit.integration.spec.ts: my raw http.request-based version and another (cleaner, fetch()-based) version from main were concatenated rather than merged — duplicate imports, an unclosed `it` block. Kept the fetch()-based version. - stellar.service.spec.ts: duplicate `error` key in a SorobanSimulationResult object literal. - transaction.service.spec.ts: my placeholder describe.skip stub was prepended to a real, correctly implemented version of the same suite that showed up on main independently. Kept the real implementation, dropped the stub.
…ues-63-64-283 feat: stream audit exports and harden payment validation
…-handling Add centralized error handling and scrubbed structured logging for background workers
…60-draft feat: address requested issues - closes ASTROIDX556#260, closes ASTROIDX556#259, closes ASTROIDX556#258, closes ASTROIDX556#257
…atch-recovery-validation-retry Add query metrics, batch retry recovery, input sanitization, and HTTP retry with backoff
feat: Add comprehensive observability, security, and simulation improvements
…57-61 feat: correlate requests, paginate audit, sign webhooks
…-264-draft feat: address requested issues - closes ASTROIDX556#264, closes ASTROIDX556#263, closes ASTROIDX556#262, closes ASTROIDX556#261
Contributor
CI Checks Failed — Action NeededThe current CI run has failing checks, so this PR remains unmergeable. Please inspect the failed jobs, fix the underlying issues, and push the correction to this PR branch. Failing checks:
I will not merge while these checks are failing. |
A duplicate db:verify script entry (missing comma) made package.json invalid JSON, causing npm ci to fail in CI with EJSONPARSE. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
RiskAssessment.transactionId is already @unique, which Prisma covers with its own index; the extra @@index([transactionId]) had no corresponding migration and tripped the CI drift check. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
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
20 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.
closes #256
Summary
Expands
scripts/verify-migrations.shinto three tiers of checks and runs it as a dedicatedverify-migrationsCI job. Every problem is reported in one run, with file and line, as GitHub annotations on the PR diff.1. Structure and SQL (always, no database needed)
prisma validateon the schema. A placeholderDATABASE_URLis used when none is set; it never connects.migration_lock.tomlis present and its provider matches the schema datasource provider.<YYYYMMDDHHMMSS>_<snake_case>(plus the0_baseline convention), with no duplicate timestamps and no stray files.migration.sqlwith at least one statement.;. It is written in POSIX awk, so it runs under mawk onubuntu-latest.2. History (
MIGRATION_BASE_REF)prisma migrate deploy.SET NOT NULL, renames) are reported as warnings. They do not fail the build.3. Replay and drift (
SHADOW_DATABASE_URL)prisma migrate diff --from-migrations ... --exit-codereplays the full history, in order, on an empty Postgres. This catches invalid SQL and a migration that depends on an object created by a later one.schema.prismahas changes that no migration captures (drift), and prints the SQL that would be needed.CI
verify-migrationsjob with its own disposable Postgres (astroid_shadow) andfetch-depth: 0.origin/<base_ref>.build-and-testis removed. It setSHADOW_DATABASE_URL, but the old script never used it.actionlint(no findings).Other changes
npm run db:verifynow runsbash scripts/verify-migrations.sh, so it works on Windows. The script is also marked executable (it was stored as 100644).CONTRIBUTING.md(new "Database migrations" section) anddocs/database.md.Testing
src/database/verify-migrations-script.spec.tsruns the script against throwaway migration trees and temporary git repos (21 cases). It covers every structural and SQL error, GitHub annotation output, and the history checks: edits, deletions, out-of-order migrations, destructive-statement warnings, and an unresolvable base ref.mainhas no drift, so the new job will be green.schema.prismafails with the missingALTER TABLE ... ADD COLUMN.npm run db:verify,npm run typecheck,npm run lint,npm test(1101 passing),npm run build.Notes for reviewers
Open PR #354 also edits
scripts/verify-migrations.sh,ci.ymlandCONTRIBUTING.md. Whichever merges second will need a rebase.