fix(lint): replace no-explicit-any with concrete types in src files - #581
RAMADANdboss wants to merge 827 commits into
Conversation
…FIXED (TegoLabs#270) * TegoLabs#145 feat(core): integrate HashiCorp Vault for key retrieval FIXED * chore(tests): split mock secrets to evade GitGuardian false positives * chore(tests): split more mock secrets to evade GitGuardian --------- Co-authored-by: AbdulmalikAlayande <114596864+AbdulmalikAlayande@users.noreply.github.com>
…FIXED (TegoLabs#270) * TegoLabs#145 feat(core): integrate HashiCorp Vault for key retrieval FIXED * chore(tests): split mock secrets to evade GitGuardian false positives * chore(tests): split more mock secrets to evade GitGuardian --------- Co-authored-by: AbdulmalikAlayande <114596864+AbdulmalikAlayande@users.noreply.github.com>
- Add docker-compose.devnet.yaml: Quickstart testing image, --limits unlimited, 30s polling cadence, debug logging, isolated named volumes - Enhance docker-compose.yaml: restart policies, JSON log rotation, parameterised ports, LOG_LEVEL/NODE_ENV env vars - Add .env.example: full environment variable reference with inline comments - Add tests/docker/devnet-compose.test.ts: 32 TDD assertions covering file presence, service config, volume isolation, network sharing, .env.example, and compose merge compatibility - Update .dockerignore: exclude compose files, systemd/, docs/, templates/ - Update .gitignore: allow .env.example via negation rule Acceptance criteria met: docker compose -f docker-compose.yaml -f docker-compose.devnet.yaml up boots daemon and mock RPC environment successfully. All 530 tests pass, 63 docker-specific tests, 5 skipped TODOs.
…des and acceptance criteria
…estimates - Add countExtensionsInLastHour() to repositories.ts to query extension_history for the past 60-minute window (issue TegoLabs#142) - Export HOURLY_RATE_LIMIT = 5 constant from extension.ts (issue TegoLabs#142) - Export isRateLimited() that gates on countExtensionsInLastHour >= limit (issue TegoLabs#142) - Enforce rate limit in runAutoExtensions(): skip + log when limit reached (issue TegoLabs#142) - Export ResourceEstimate interface and parseResourceEstimate() in rpc/client.ts to extract cpuInstructions, memoryBytes, minResourceFee from simulation responses (issue TegoLabs#133) - Add comprehensive TDD tests written before implementation: - tests/db/rate_limiter.test.ts: countExtensionsInLastHour edge cases - tests/core/rate_limiter.test.ts: isRateLimited, runAutoExtensions integration - tests/rpc/resource_estimate.test.ts: parseResourceEstimate + failure edge cases Closes TegoLabs#133 Closes TegoLabs#137 Closes TegoLabs#142
vitest.config.ts only globs tests/**/*.test.ts, so this file was never executed despite being valid, passing coverage for the exact dispatch/retry/channel-routing logic about to be refactored to support pluggable alert channels.
Central registration point for alert channel plugins. A contributor adding a new channel calls registerAlertChannel() with a ChannelDefinition instead of editing dispatcher.ts's channel map, the CLI's --type if/else chain, and a DB CHECK constraint.
Preserves existing behavior exactly: same target flags, same missing- target error text, same lazy dynamic import for discord/telegram, same webhook-only HMAC signing. This is the reference implementation new channel plugins should follow.
Replaces the hardcoded DEFAULT_CHANNELS object with a registry-backed lookup, so a plugin channel registered anywhere becomes deliverable without editing this file. Explicit channels overrides (used throughout the test suite) are unaffected — only the default when one is omitted changed source. deliverSingleAlert's channelType is widened from a fixed union to string for the same reason.
channel_type validity is now enforced by the alert channel registry at the application layer instead of a fixed SQL enum, so adding a channel no longer requires a schema change. The CHECK now only guards against an empty string.
…ration The SCHEMA comment-stripper (`--.*\n`) silently failed to match comments ending in \r\n, since JS's `.` excludes all line terminators including \r. On a CRLF checkout, an unstripped comment survives into the whitespace-collapsed script, and SQLite's own -- comment then runs to the string's end, swallowing every statement after it with no thrown error. Switched to `--[^\n]*\n`, which matches either line ending. Latent since schema.sql had no comments before this change. Also adds relaxChannelTypeChecks(), following the existing migrateAlertConfigsChannelTypeCheck() convention, to rebuild alert_configs and resource_alert_configs in place for databases created before the CHECK was relaxed.
AlertConfig, UndeliveredAlert, ResourceAlertConfig, and UndeliveredResourceAlerts previously hardcoded the built-in channel names in their type signatures. The registry is now the source of truth for valid channel names, so these widen to string.
The beforeEach block manually rebuilt alert_configs with a hardcoded 5-name CHECK on every test, a leftover workaround from before schema.sql had these columns natively. It silently undid the CHECK relaxation, since it ran unconditionally rather than detecting whether schema.sql already had the change. getDatabaseForTesting() already execs the current schema.sql into a fresh database, so the whole block was redundant even before this. Also adds coverage for plugin channel_type values and empty-string rejection on both alert_configs and resource_alert_configs.
Replaces the per-channel if/else chain with a lookup against the alert channel registry, so a plugin channel's --type, target flag, missing-target error, and signing behavior all come from its ChannelDefinition instead of a hardcoded branch in this file. All existing error message text is preserved exactly for the five built-in channels; the generic "unknown type" message is now built from whatever channels are actually registered.
- discord.ts: payload typed as Record<string,unknown> - keys.ts: add KeytarLike interface, remove any from keytar field - pagerduty.ts: payload typed as Record<string,unknown>, safe inner casts - slack.ts: introduce SlackBlock type alias, remove any[] and any fields - schemaFormatter.ts: applySchema/loadAndApplySchema params typed as unknown - inspect.ts: catch error: unknown, statusIndicator cast to TTLStatus - mcp.ts: catch error: unknown - pause.ts: catch error: unknown - resume.ts: catch error: unknown - watch.ts: catch error: unknown in watch and unwatch handlers - aws_secrets.ts: clientPromise Promise<unknown>, clientConfig Record<string,unknown> - channels.ts: replace client as any with typed cast, remove b: any - decoder.ts: symbol typed as unknown in interface and local variable - discovery.ts: health/request/page casts replaced with narrow types
|
@RAMADANdboss 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! 🚀 |
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis PR replaces explicit ChangesType safety cleanup
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/core/decoder.ts (1)
22-30: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd edge-case coverage for the widened
symbolcontract.Verify tests cover contract-instance keys, non-string native values from
scValToNative, and the exception path returning"Unknown".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/decoder.ts` around lines 22 - 30, Add edge-case tests for the decoder logic assigning symbol: verify contract-instance keys produce "ContractInstance", non-string values returned by scValToNative are preserved, and exceptions from scValToNative produce "Unknown".src/alerts/slack.ts (1)
135-146: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winValidate custom Slack blocks before assigning them to the payload.
JSON.parse()is still untrusted here:parsed.blockscan be non-array/malformed, and whenparsedis an array the currentSlackBlock[]type allows anyRecord<string, unknown>rather than real Slack block objects. Parse asunknown, validate the array and at least atypeblock shape, and fall back to text when invalid.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/alerts/slack.ts` around lines 135 - 146, Update the customMessage parsing in the Slack payload construction to parse into unknown and validate parsed blocks before assigning them. Require arrays to contain valid Slack block objects with at least a string type field, validate parsed.blocks similarly for object payloads, and fall back to buildFallbackText(event) when validation fails while preserving valid text and blocks handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/alerts/keys.ts`:
- Around line 18-20: Update the constructor of the keytar-owning class to ensure
this.keytar is always initialized: either require and validate a KeytarLike
dependency or create the appropriate real keytar fallback when keytarMock is
omitted. Remove the unsafe cast that masks undefined, while preserving mock
injection for tests and ensuring saveKey() and listKeys() can safely dereference
this.keytar.
In `@src/alerts/pagerduty.ts`:
- Line 123: Update buildPayload and the send flow to model the PagerDuty
envelope with an explicit type, including the nested payload and guarded
template overrides, instead of casting the result to Record<string, unknown>.
Remove or narrow the top-level and nested casts before mutation while preserving
the existing payload construction and send behavior.
In `@src/commands/inspect.ts`:
- Line 74: Update the rendering expression in the inspect output around
statusIndicator so item.status is validated as a TTLStatus before use, rather
than relying on the direct type assertion. Add or reuse a runtime guard that
handles invalid values safely, while preserving statusIndicator behavior for
valid statuses.
In `@src/core/aws_secrets.ts`:
- Line 9: Update the cached client declaration in the AWS Secrets Manager class
to use Promise<SecretsManagerClient> rather than Promise<unknown>. Type the
constructor configuration with the SecretsManager client’s indexed/config type
or a typed factory, then adjust resolveKey() to call client.send(...) directly
without an InstanceType cast and remove the Record<string, unknown>
configuration typing.
---
Outside diff comments:
In `@src/alerts/slack.ts`:
- Around line 135-146: Update the customMessage parsing in the Slack payload
construction to parse into unknown and validate parsed blocks before assigning
them. Require arrays to contain valid Slack block objects with at least a string
type field, validate parsed.blocks similarly for object payloads, and fall back
to buildFallbackText(event) when validation fails while preserving valid text
and blocks handling.
In `@src/core/decoder.ts`:
- Around line 22-30: Add edge-case tests for the decoder logic assigning symbol:
verify contract-instance keys produce "ContractInstance", non-string values
returned by scValToNative are preserved, and exceptions from scValToNative
produce "Unknown".
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 833d4529-153d-4289-b3c0-b9821207a5db
📒 Files selected for processing (14)
src/alerts/discord.tssrc/alerts/keys.tssrc/alerts/pagerduty.tssrc/alerts/slack.tssrc/cli/schemaFormatter.tssrc/commands/inspect.tssrc/commands/mcp.tssrc/commands/pause.tssrc/commands/resume.tssrc/commands/watch.tssrc/core/aws_secrets.tssrc/core/channels.tssrc/core/decoder.tssrc/core/discovery.ts
📜 Review details
🔇 Additional comments (13)
src/alerts/keys.ts (1)
6-17: LGTM!Also applies to: 35-35
src/alerts/slack.ts (1)
15-17: LGTM!Also applies to: 43-43
src/alerts/discord.ts (1)
189-189: LGTM!src/core/channels.ts (1)
112-115: LGTM!src/core/discovery.ts (2)
74-74: LGTM!Also applies to: 120-121
93-106: 🎯 Functional CorrectnessNo change needed for the
getEventslimit.
@stellar/stellar-sdkacceptslimitonGetEventsRequestand maps it into the RPCpagination.limitfield, so the current top-level shape is still sent to the RPC server through the SDK; usingpagination: { limit: 100 }directly is only an optional cleanup.> Likely an incorrect or invalid review comment.src/core/decoder.ts (1)
5-5: LGTM!src/cli/schemaFormatter.ts (1)
74-76: LGTM!Also applies to: 85-88
src/commands/inspect.ts (1)
79-84: LGTM!src/commands/mcp.ts (1)
15-17: LGTM!src/commands/pause.ts (1)
28-32: LGTM!src/commands/resume.ts (1)
27-31: LGTM!src/commands/watch.ts (1)
160-165: LGTM!Also applies to: 209-213
| constructor(keytarMock?: KeytarLike) { | ||
| // Fallback to allow mock injection during test environments safely | ||
| this.keytar = keytarMock; | ||
| this.keytar = keytarMock as KeytarLike; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Make the keytar dependency truly required or initialize a real fallback.
keytarMock may be undefined, but the cast at Line 20 hides that fact. A no-argument construction will crash when saveKey() or listKeys() dereferences this.keytar.
Proposed fix
- constructor(keytarMock?: KeytarLike) {
- this.keytar = keytarMock as KeytarLike;
+ constructor(keytar: KeytarLike) {
+ this.keytar = keytar;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| constructor(keytarMock?: KeytarLike) { | |
| // Fallback to allow mock injection during test environments safely | |
| this.keytar = keytarMock; | |
| this.keytar = keytarMock as KeytarLike; | |
| constructor(keytar: KeytarLike) { | |
| // Fallback to allow mock injection during test environments safely | |
| this.keytar = keytar; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/alerts/keys.ts` around lines 18 - 20, Update the constructor of the
keytar-owning class to ensure this.keytar is always initialized: either require
and validate a KeytarLike dependency or create the appropriate real keytar
fallback when keytarMock is omitted. Remove the unsafe cast that masks
undefined, while preserving mock injection for tests and ensuring saveKey() and
listKeys() can safely dereference this.keytar.
|
|
||
| const customMessage = renderAlertTemplate("pagerduty", event); | ||
| const payload = buildPayload(event) as any; | ||
| const payload = buildPayload(event) as Record<string, unknown>; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate and inspect the relevant file and symbols without running repo code.
if [ -f src/alerts/pagerduty.ts ]; then
echo "== file exists =="
wc -l src/alerts/pagerduty.ts
echo
echo "== outline =="
ast-grep outline src/alerts/pagerduty.ts --view expanded || true
echo
echo "== relevant lines 1-190 =="
sed -n '1,190p' src/alerts/pagerduty.ts | nl -ba | sed -n '1,190p'
else
echo "src/alerts/pagerduty.ts not found"
git ls-files | rg 'pagerduty|alerts' || true
fiRepository: AbdulmalikAlayande/sorokeep
Length of output: 1137
🏁 Script executed:
#!/bin/bash
set -euo pipefail
if [ -f src/alerts/pagerduty.ts ]; then
echo "== file exists =="
wc -l src/alerts/pagerduty.ts
echo
echo "== outline =="
ast-grep outline src/alerts/pagerduty.ts --view expanded || true
echo
echo "== relevant lines 1-190 =="
sed -n '1,190p' src/alerts/pagerduty.ts | nl -ba | sed -n '1,190p'
else
echo "src/alerts/pagerduty.ts not found"
git ls-files | rg 'pagerduty|alerts' || true
fiRepository: AbdulmalikAlayande/sorokeep
Length of output: 1137
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,200p' src/alerts/pagerduty.tsRepository: AbdulmalikAlayande/sorokeep
Length of output: 7812
Avoid untyped mutations of the PagerDuty payload.
buildPayload() returns the full PagerDuty envelope as unknown, and send() casts the top-level record plus nested payload before mutation. Model the envelope inline (for example, with a typed payload and any guarded template overrides) so these casts can be removed or narrowed.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/alerts/pagerduty.ts` at line 123, Update buildPayload and the send flow
to model the PagerDuty envelope with an explicit type, including the nested
payload and guarded template overrides, instead of casting the result to
Record<string, unknown>. Remove or narrow the top-level and nested casts before
mutation while preserving the existing payload construction and send behavior.
| } | ||
| console.log( | ||
| ` TTL: ${item.remainingTTL.toLocaleString()} ledgers (${item.approximateTimeRemaining}) ${statusIndicator(item.status as any)}`, | ||
| ` TTL: ${item.remainingTTL.toLocaleString()} ledgers (${item.approximateTimeRemaining}) ${statusIndicator(item.status as "ok" | "warning" | "critical" | "expired")}`, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
Avoid asserting an unvalidated status as TTLStatus.
This cast can let out-of-contract values reach statusIndicator; its exhaustive switch then produces undefined for an invalid runtime status. Type item.status as TTLStatus at the result boundary or add a runtime guard before rendering.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/commands/inspect.ts` at line 74, Update the rendering expression in the
inspect output around statusIndicator so item.status is validated as a TTLStatus
before use, rather than relying on the direct type assertion. Add or reuse a
runtime guard that handles invalid values safely, while preserving
statusIndicator behavior for valid statuses.
| private region: string; | ||
| private profile?: string; | ||
| private clientPromise?: Promise<any>; | ||
| private clientPromise?: Promise<unknown>; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Repository files around aws_secrets:"
fd -a 'aws_secrets\.ts$|package.json$|.*secrets.*' . | sed 's#^\./##' | head -100
echo
echo "File outline:"
ast-grep outline src/core/aws_secrets.ts --view expanded || true
echo
echo "Relevant file contents:"
cat -n src/core/aws_secrets.ts
echo
echo "Search SecretsManagerClient and related usages:"
rg -n "SecretsManagerClient|clientPromise|clientConfig|as InstanceType|Record<string, unknown>" -S .Repository: AbdulmalikAlayande/sorokeep
Length of output: 8266
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "package manager files:"
git ls-files | rg '(^|/)(package.json|package-lock.json|pnpm-lock.yaml|yarn.lock|bun.lockb|tsconfig.json)$' || true
echo
echo "package.json deps:"
if [ -f package.json ]; then
node -e "const p=require('./package.json'); console.log(JSON.stringify({dependencies:p.dependencies,devDependencies:p.devDependencies}, null, 2))" | rg -n '"(`@aws-sdk/client-secrets-manager`|`@aws-sdk/credential-providers`|typescript)"|("aws-sdk|aws-crt|`@smithy`|`@aws-crypto`)"' | head -80 || true
fi
echo
echo "Lockfile entries for AWS SDK (if present):"
rg -n '`@aws-sdk/client-secrets-manager`|`@aws-sdk/credential-providers`|`@aws-sdk/signature-v4`|`@aws-sdk/config-resolver`|types' package-lock.json pnpm-lock.yaml yarn.lock 2>/dev/null | head -120 || true
echo
echo "TypeScript config:"
if [ -f tsconfig.json ]; then cat -n tsconfig.json; fiRepository: AbdulmalikAlayande/sorokeep
Length of output: 10144
Preserve the AWS Secrets Manager client type instead of casting through unknown.
Type the cached promise as Promise<SecretsManagerClient> and use an indexed type or typed factory for the constructor config, so resolveKey() can call client.send(...) without an InstanceType cast and clientConfig is no longer hidden behind Record<string, unknown>.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/core/aws_secrets.ts` at line 9, Update the cached client declaration in
the AWS Secrets Manager class to use Promise<SecretsManagerClient> rather than
Promise<unknown>. Type the constructor configuration with the SecretsManager
client’s indexed/config type or a typed factory, then adjust resolveKey() to
call client.send(...) directly without an InstanceType cast and remove the
Record<string, unknown> configuration typing.
|
| GitGuardian id | GitGuardian status | Secret | Commit | Filename | |
|---|---|---|---|---|---|
| - | - | Generic High Entropy Secret | ded54f4 | tests/commands/guard-cli-export-import.test.ts | View secret |
🛠 Guidelines to remediate hardcoded secrets
- Understand the implications of revoking this secret by investigating where it is used in your code.
- Replace and store your secret safely. Learn here the best practices.
- Revoke and rotate this secret.
- If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.
To avoid such incidents in the future consider
- following these best practices for managing and storing secrets including API keys and other credentials
- install secret detection on pre-commit to catch secret before it leaves your machine and ease remediation.
🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.
43ad363 to
8692701
Compare
…581, #363) Type-only sweep across the alert channels (discord/pagerduty/slack payloads, keys.ts's KeytarLike interface), CLI schema formatter, five command catch blocks, and RPC-adjacent core modules (aws_secrets, channels, decoder, discovery). Drops the lint warning count from 264 to 237 with zero new tsc errors. Fixes one real bug surfaced by the tightening: discovery.ts's cursor-based event pagination was building { pagination: { cursor } }, which doesn't exist on the SDK's GetEventsRequest type — the real shape is a top-level `cursor: string` field, mutually exclusive with startLedger/endLedger. Under `any` this was silently accepted and pagination beyond the first 100 events per contract likely never worked. No test harness exercises multi-page getEvents responses, so this is verified structurally (tsc against the real SDK type) rather than behaviorally — flagging here for visibility.
|
Thanks — good, well-scoped set of fixes. Applied all 14 files (your branch predated the budget_exhausted alert event, so I reconciled discord.ts/pagerduty.ts/slack.ts by hand to keep both). Two follow-ups needed to actually compile against current dependency versions: aws_secrets.ts's SecretsManagerClient constructor cast needed a direct SecretsManagerClientConfig type instead of ConstructorParameters<...>[0] (which resolved to a union including undefined), and discovery.ts's getEvents request needed a real fix, not just a cast — it turned out |
What does this PR do?
Why?
Does this touch secret-key handling or transaction submission?
Checklist
npm test)npx tsc --noEmit)npm run lint)console.login core logiccloses #363