feat: add guard script to detect misplaced test files - #533
marvelousjeremiah24 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.
- Add scripts/check-test-file-locations.mjs that globs **/*.test.ts and exits non-zero if any match falls outside tests/, excluding node_modules/, dist/, and .npm-cache/ - Add npm run check:test-locations script - Add CI step between Lint and Run Tests to fail fast on misplaced files - Add 14 tests in tests/scripts/check-test-file-locations.test.ts covering clean state (exit 0), misplaced files (exit 1), exclusions, output content, and real-repo integration Fixes the silent-exclusion bug where src/alerts/*.test.ts went unexecuted because vitest only includes tests/**/*.test.ts.
|
@marvelousjeremiah24 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
WalkthroughAdds a Node.js guard that detects misplaced ChangesTest Location Guard
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 Warning |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 @.github/workflows/ci.yml:
- Around line 43-44: Relocate the misplaced src/alerts/*.test.ts files into
tests/ and update their imports as needed so the check:test-locations gate
passes. In .github/workflows/ci.yml lines 43-44, retain the Check Test File
Locations pre-test gate; in tests/scripts/check-test-file-locations.test.ts
lines 174-180, update the repository integration assertion to expect exit code 0
after the relocation.
🪄 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: 5641efaa-06e5-4abb-b255-6a08a0df205d
📒 Files selected for processing (4)
.github/workflows/ci.ymlpackage.jsonscripts/check-test-file-locations.mjstests/scripts/check-test-file-locations.test.ts
📜 Review details
🧰 Additional context used
🪛 ast-grep (0.45.0)
tests/scripts/check-test-file-locations.test.ts
[warning] 7-7: Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { spawnSync } from "node:child_process";
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
🔇 Additional comments (2)
scripts/check-test-file-locations.mjs (1)
18-72: LGTM!package.json (1)
27-27: LGTM!
| - name: Check Test File Locations | ||
| run: npm run check:test-locations |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Clean the existing misplaced tests before enabling this gate.
The integration test explicitly confirms that this command fails at the repository root because of src/alerts/*.test.ts; adding it here therefore makes CI fail on every run. Move those tests under tests/ (updating imports as needed) and make the repository integration test expect exit code 0.
.github/workflows/ci.yml#L43-L44: retain this pre-test gate only with a repository state that passes it.tests/scripts/check-test-file-locations.test.ts#L174-L180: after relocating the existing files, assert successful execution rather than the known failing state.
📍 Affects 2 files
.github/workflows/ci.yml#L43-L44(this comment)tests/scripts/check-test-file-locations.test.ts#L174-L180
🤖 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 @.github/workflows/ci.yml around lines 43 - 44, Relocate the misplaced
src/alerts/*.test.ts files into tests/ and update their imports as needed so the
check:test-locations gate passes. In .github/workflows/ci.yml lines 43-44,
retain the Check Test File Locations pre-test gate; in
tests/scripts/check-test-file-locations.test.ts lines 174-180, update the
repository integration assertion to expect exit code 0 after the relocation.
|
| 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
scripts/check-test-file-locations.mjs globs the whole repo tree for *.test.ts (excluding node_modules/dist/.npm-cache) and exits non-zero if any live outside tests/ — exactly the class of bug that let 10+ src/alerts/*.test.ts files sit unexecuted, undetected, for an unknown stretch of this wave. Wired into CI as a new step between Lint and Run Tests, npm script "check:test-locations". Two fixes over the submitted version: the script's error output used the OS path separator, so on Windows it printed backslash paths that a forward-slash-matching test (and any Windows contributor reading the output) wouldn't recognize — now always reports forward-slash paths. The real-repo integration test asserted the old, pre-cleanup failure state (10 misplaced files); flipped to assert success, since src/ is now actually clean of orphaned test files.
|
Thanks — good script design and thorough test coverage. Applied all 4 files. Two fixes needed: the script's misplaced-path output used the OS path separator, so on Windows it printed backslash paths that your own forward-slash-matching test (and any Windows contributor) wouldn't recognize — now always reports forward-slash paths regardless of OS. And the real-repo integration test asserted the old failure state (10 misplaced src/alerts/*.test.ts files) — flipped to assert success now that the orphaned-file cleanup issues in this phase have actually landed, per your own issue's note to pick this one up last. Merged as 51c8995. |
18 participants
Fixes the silent-exclusion bug where src/alerts/.test.ts went unexecuted because vitest only includes tests/**/.test.ts.
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 logic