Skip to content

chore(provider): code-quality tidy-up (demo, for review) - #3257

Draft
HughParry wants to merge 2 commits into
mainfrom
chore/provider-code-tidy
Draft

HughParry wants to merge 2 commits into
mainfrom
chore/provider-code-tidy

Conversation

@HughParry

Copy link
Copy Markdown
Contributor

Demo — not for merging yet. This is an example of a code-quality tidy-up on one package (@prosopo/provider), opened for my review. Not something to get merged ASAP.

Currently needs human review on workflow.

What changed

Only files in packages/provider/src (not tests). 67 files, about 1,900 lines removed and 900 added.

Comments

  • Removed comments that just repeat the code (// Check if email is spam).
  • Removed or rewrote comments that tell the history of the code instead of what it does now ("previously…", "until this branch was added", "Historical shape (removed 2026-08-06)", incident stories).
  • Where the same explanation was pasted in several places, it now lives in one.
  • Fixed comments that were wrong, e.g. the checkForHardBlock doc (said it only checks Block policies), backgroundBuffer.take() (said it returns null), the hardBlockVerdictCache race note (said a race was impossible), and a doc comment attached to the wrong function in forwardVerify.ts.
  • Kept comments that explain why: security reasoning, constraints, workarounds.

Code (small, same-file only)

  • Repeated blocks folded into local helpers:
    • persistRejection in puzzle verify (6 copies)
    • serveImageCaptcha in the frictionless decision machine (5 copies)
    • writeSubmissionResult / rejectSubmission in image solution submit
    • invalidateCachedSessionAndHash in captchaManager
    • cacheSessionRecord in frictionlessTasks
    • fetchAndCacheArtifact / pickByPriority in decisionMachineRunner
    • the ipInfo lookups in the frictionless handler (computed once instead of 9 times)
  • Removed unused imports, unused variables, one unused private function (isValidPrivateKey), redundant casts and pointless temporary variables.
  • A few clearer local names (tempsiteSettings, lConfiglanguagePenalties).

What effect it has

None intended. Exports, HTTP responses, status codes, error keys and log messages are unchanged. Nothing moved between files.

How it was checked

  • Unit tests: all 1,300 provider unit tests pass, the same as on main before the change. No tests were added or changed.
  • Type check: tsc --noEmit on provider reports 0 errors, same as before. (build:tsc fails on main for an unrelated reason: a stale redis install breaks redis-client.)
  • Lint: biome is clean.
  • Comment-only files: a script stripped comments and types from the old and new version of every file and compared the output. 32 of the 67 files produce identical JavaScript.
  • Code-change files: I read the diff of each of the other 35 by hand.
  • Integration tests: not run.

Noticed while tidying, deliberately not changed

These look like real bugs or smells. Worth separate PRs:

  • Puzzle email checks:
    • The spam-email-domain and email-count rejections in puzzle verify update the puzzle record but not the session, unlike every other rejection (puzzleTasks.ts).
    • Puzzle verify never runs evaluateEmailSpamRules, so per-site email rules don't apply to puzzle captchas.
  • Dataset check: datasetTasksUtils.ts checks the solved count against minUnsolvedCaptchas.
  • Hard-block cache: the cache key in hardBlockVerdictCache leaves out webBotAuthAgent, so a verified bot and an unverified request with the same IP/JA4/UA share a cached rule list for up to 10s.
  • Authenticated fast-path in the frictionless handler:
    • It creates the session before checking validation.valid.
    • It passes the raw token (possibly "") instead of sessionToken.
  • Undecoded field: decryptPayload never returns dz, although it is decoded and stored elsewhere.
  • Missing metric: the autoBan branch in accessPolicy.ts doesn't record the auto_ban_score metric that decisionMachine.ts does.
  • Session IP check: validateSessionIP always returns valid, so the currentIP branch in isValidRequest can never fail.
  • Unreachable code:
    • if (!solution) in image verify, because getDappUserCommitmentById throws first.
    • The !background throw in puzzleRenderer.
  • PoW provided IP: PoW only stores providedIp when there was no earlier failure; image and puzzle always store it.

Remove comments that restate the code or narrate its history, keep each explanation in one place, correct comments that no longer match the code, and fold repeated blocks within a file into small local helpers. No behavioural change intended.
# Conflicts:
#	packages/provider/src/tasks/detection/getBotScore.ts
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant