fix(review): hard-block Voyage and Firecrawl secrets by JSONbored · Pull Request #3980 · JSONbored/loopover

JSONbored · 2026-07-07T08:32:34Z

Motivation

Ensure parity between the REES advisory secret scanner and the repository's deterministic PR hard-blocking path so newly-detected high-confidence credentials cannot be merged when enrichment is disabled.
Close a security-control gap where voyage_api_key / firecrawl_api_key were detected only by optional enrichment and not by the unconditional secret_leak gate.

Description

Add voyage_api_key and firecrawl_api_key regex patterns to the deterministic scanner in src/review/secrets-scan.ts using the same length and continuation boundaries as the REES rules.
Add "voyage_api_key" and "firecrawl_api_key" to the hard-block allowlist HARD_SECRET_KINDS in src/review/safety.ts so secretLeakFinding produces the secret_leak blocker for those matches.
Add unit tests in test/unit/secrets-scan.test.ts covering positive matches and negative cases (length floor and hyphen/underscore continuation) and extend test/unit/safety-wiring.test.ts to assert gate hard-block parity for the new kinds.

Testing

Ran git diff --check which produced no whitespace/conflict errors.
Executed the unit tests with npx vitest run test/unit/secrets-scan.test.ts test/unit/safety-wiring.test.ts and those tests passed.
Ran the TypeScript typecheck with npm run typecheck and it passed.
Attempted coverage run npx vitest run --coverage test/unit/secrets-scan.test.ts test/unit/safety-wiring.test.ts which executed the tests but failed during coverage remapping with TypeError: jsTokens is not a function in ast-v8-to-istanbul; the failure is in coverage post-processing, not in the test assertions themselves.

Codex Task

superagent-security · 2026-07-07T08:33:34Z

Superagent didn't find any vulnerabilities or security issues in this PR.

loopover-orb · 2026-07-07T08:39:03Z

Caution

🟥🟥🟥🟥🟥🟥🟥🟥🟥🟥🟥🟥

🛑 Gittensory review result - fixes required

_{Review updated: 2026-07-07 10:46:32 UTC}

4 files · 1 AI reviewer · 3 blockers · readiness 93/100 · CI failing · blocked

🛑 Suggested Action - Manual Review

No linked issue detected — If this PR is intended to solve an issue, link it explicitly in the PR body.
Maintainer requires a linked issue — Link the relevant issue (for example Closes #123) before opening the PR.
AI reviewers agree on a likely critical defect: src/review/secrets-scan.ts:32 — the `voyage_api_key` pattern `\b(?:pa|al)-[A-Za-z0-9]{20,}` uses generic 2-char prefixes with no other distinguishing structure (unlike `ghp_`, `AKIA`, `glpat-`, `hf_`, etc.), so any ordinary kebab-case identifier, URL slug, or CSS/config string starting with `pa-` or `al-` followed by 20+ alnum chars will hard-block a PR via the unconditional `secret_leak` gate — this needs a narrower pattern or an explicit justification given the file's own documented history of false-positive hard-blocks from weak heuristics. — Resolve the flagged defect, or override if the AI reviewers are mistaken, then re-run the gate.

Review summary
This PR adds `voyage_api_key` and `firecrawl_api_key` detection to the deterministic scanner (secrets-scan.ts) and wires both into the unconditional hard-block set (safety.ts HARD_SECRET_KINDS), closing the gap where these two credential types were only caught by optional REES enrichment. The mechanical wiring is correct and well-tested for length-floor/continuation-boundary edge cases, and typecheck/unit tests pass per the PR description (the coverage-run failure is a tooling issue in ast-v8-to-istanbul, not a test failure). My one real concern is that the `voyage_api_key` regex uses generic 2-letter prefixes (`pa-`/`al-`) that are far weaker than every sibling pattern in this file, which risks false-positive hard-blocks on ordinary hyphenated identifiers — the exact failure mode this file's own header comments say already burned this repo (RC6, #1505/#1495/#1485).

Blockers

src/review/secrets-scan.ts:32 — the `voyage_api_key` pattern `\b(?:pa|al)-[A-Za-z0-9]{20,}` uses generic 2-char prefixes with no other distinguishing structure (unlike `ghp_`, `AKIA`, `glpat-`, `hf_`, etc.), so any ordinary kebab-case identifier, URL slug, or CSS/config string starting with `pa-` or `al-` followed by 20+ alnum chars will hard-block a PR via the unconditional `secret_leak` gate — this needs a narrower pattern or an explicit justification given the file's own documented history of false-positive hard-blocks from weak heuristics.

Nits — 5 non-blocking

src/review/secrets-scan.ts:32 — bundling two unrelated services (Voyage AI's `pa-` and MongoDB Atlas's `al-`) under one `voyage_api_key` kind name is confusing; consider splitting into `voyage_api_key` and `mongodb_atlas_key` for clearer findings/detail text.
test/unit/secrets-scan.test.ts — the new tests cover the length floor and continuation-boundary but don't cover a plausible false-positive shape (e.g. a realistic kebab-case identifier), which would have surfaced the prefix-specificity concern above.
External brief flags magic numbers (20, 16) in the new regexes; consider named constants (e.g. `VOYAGE_KEY_MIN_LEN`) matching the style already used for other length floors in this file, though this is purely cosmetic.
Tighten `voyage_api_key`'s prefix requirement (e.g. require a longer fixed-format body, or drop the 2-letter `al-`/`pa-` prefixes in favor of something more specific to each service) before treating it as an unconditional hard blocker.
If keeping the current pattern, add a code comment next to `voyage_api_key` explaining why the weaker prefix was accepted despite the file's existing guidance to avoid short/generic heuristics on the hard-block path.

Why this is blocked

src/review/secrets-scan.ts:32 — the `voyage_api_key` pattern `\b(?:pa|al)-[A-Za-z0-9]{20,}` uses generic 2-char prefixes with no other distinguishing structure (unlike `ghp_`, `AKIA`, `glpat-`, `hf_`, etc.), so any ordinary kebab-case identifier, URL slug, or CSS/config string starting with `pa-` or `al-` followed by 20+ alnum chars will hard-block a PR via the unconditional `secret_leak` gate — this needs a narrower pattern or an explicit justification given the file's own documented history of false-positive hard-blocks from weak heuristics.
No linked issue detected — If this PR is intended to solve an issue, link it explicitly in the PR body.
Maintainer requires a linked issue — Link the relevant issue (for example Closes #123) before opening the PR.

CI checks failing

validate
validate-code

Signal	Result	Evidence
Code review	❌ 3 blockers	1 reviewer
Linked issue	⚠️ Missing	No linked issue or no-issue rationale found.
Related work	✅ No active overlap found	No same-issue or scoped active PR overlap found.
Change scope	✅ 20/20	Low review scope from cached public metadata (no linked issue context).
Validation posture	✅ 25/25	PR body includes validation/test evidence.
Contributor workload	✅ 10/10	Author activity: 51 registered-repo PR(s), 43 merged, 569 issue(s).
Contributor context	✅ Confirmed Gittensor contributor	JSONbored; Gittensor profile; 51 PR(s), 569 issue(s).
Gate result	❌ Blocking	Repo-configured hard blocker found.

Review context

Author: JSONbored
Role context: owner (maintainer lane)
Public audience mode: oss maintainer
Lane context: Repository is configured for direct PR review.
Public profile languages: Python, TypeScript, JavaScript, Ruby, Go, Kotlin, MDX, Shell
Official Gittensor activity: 51 PR(s), 569 issue(s).
PR-specific overlap: none found.

Contributor next steps

Treat this as maintainer-lane context rather than normal contributor-lane activity.
Explain no-issue PR.
Link the issue being solved, or explicitly explain why this is a no-issue PR.

Signal definitions

Related work = same linked issue, overlapping active PRs, or title/path similarity.
Change scope = cached public metadata such as size labels, draft state, and review-burden hints.
Validation posture = whether the PR provides enough public validation/test evidence for maintainer review.
Contributor workload = public contributor activity and cleanup pressure, not a repo-wide quality failure.
Contributor context = public GitHub/Gittensor identity context; non-Gittensor status is not a blocker.

_{🟩 Safe / merged · 🟦 Advisory · 🟨 Held for review · 🟥 Blocked / closed}

💰 Earn for open-source contributions like this. Gittensor lets GitHub contributors earn for the work they already do — register to start earning →.

Checked by Gittensory, a quiet PR intelligence layer for OSS maintainers.

Re-run Gittensory review

codecov · 2026-07-07T08:42:04Z

❌ 51 Tests Failed:

Tests completed	Failed	Passed	Skipped
10423	51	10372	7

View the top 3 failed test(s) by shortest run time

test/unit/mcp-server-telemetry.test.ts > MCP server telemetry > records session-scoped MCP request errors without a tool name

Stack Traces | 0.0131s run time

Error: Failed to resolve entry for package "@jsonbored/gittensory-engine". The package may have incorrect main/module/exports specified in its package.json.
 ❯ .../gittensory-miner/lib/opportunity-fanout.js:2:1
 ❯ src/mcp/find-opportunities.ts:11:1

test/unit/mcp-server-telemetry.test.ts > MCP server telemetry > falls back when Hono does not expose an execution context

Stack Traces | 0.0268s run time

Error: Failed to resolve entry for package "@jsonbored/gittensory-engine". The package may have incorrect main/module/exports specified in its package.json.
 ❯ .../gittensory-miner/lib/opportunity-fanout.js:2:1
 ❯ src/mcp/find-opportunities.ts:11:1

test/unit/mcp-server-telemetry.test.ts > MCP server telemetry > records sanitized error telemetry when the MCP transport handler throws

Stack Traces | 0.184s run time

Error: Failed to resolve entry for package "@jsonbored/gittensory-engine". The package may have incorrect main/module/exports specified in its package.json.
 ❯ .../gittensory-miner/lib/opportunity-fanout.js:2:1
 ❯ src/mcp/find-opportunities.ts:11:1

View the full list of 48 ❄️ flaky test(s)

test/integration/api.test.ts > test/integration/api.test.ts