Imported from rios0rios0/sherif-search (.github/skills/code-review/SKILL.md). Install upstream with npx skills add rios0rios0/sherif-search --skill code-review. Copyright stays with the author.

Code review — sherif-search

sherif-search queries Google, Yahoo, Bing and Baidu, filters by domain and language, and extracts result links. Almost all of it lives in one ~1100-line unit, USS.pas, with a threaded search routine and a TIEParser callback pulling <a href> anchors. It is a preserved archive, discontinued in 2013.

When to use this skill

Use it whenever you are asked to review a pull request, a diff, a branch, or staged changes in this repository — and before opening a pull request of your own, as a self-check. It is a review skill: it produces findings, not commits.

Source of truth

The canonical engineering standards live in the rios0rios0/guide wiki. This file is a repo-tailored index into that guide plus the rules that only apply here. Precedence, highest first:

1. This repository's .github/copilot-instructions.md, CLAUDE.md, and CONTRIBUTING.md — they describe this codebase and its load-bearing invariants.
2. The rios0rios0/guide wiki — the shared standard.
3. General language idiom.

When the guide and a general convention disagree, the guide wins. When this file and the guide disagree, the guide wins and this file should be corrected in the same pull request.

Guide pages that apply here

How to run the review

1. Establish the range. Resolve the default branch with git symbolic-ref refs/remotes/origin/HEAD (strip refs/remotes/origin/; fall back to main), then read the diff with git diff <default>...HEAD and the file list with git diff <default>...HEAD --name-only.
2. Read whole files, not just hunks. A hunk cannot show a layering violation, a missing test, or a duplicated helper. Open every changed file in full, plus the files it imports from the layer below.
3. Check the change set as a unit — not only the code. A change that alters behaviour, configuration, or architecture is incomplete without its changelog entry and its documentation update, and that omission is a finding in its own right.
4. Map every finding to a rule. Each finding must name the rule it breaks and link the guide page (or the repository file) that states it. A comment that cannot be traced to a rule is a suggestion, not a defect — label it as such.
5. Report, do not rewrite. Produce the review in the output format below. Only edit files when the request explicitly asks for fixes.

What matters most in sherif-search

These are the checks that catch real defects in this repository. Work through them before the generic ones.

• ThreadSearch runs off the main thread and the VCL is not thread-safe. Any UI update from the worker must be synchronised; a direct control write from the thread is a real crash, not a style point. Critical.
• WinINet handles must be closed on every path. CheckConexao (InternetGetConnectedState), CheckUrl (InternetOpenUrl/HttpQueryInfo), and the fetch routines each open handles — an early return that skips the close leaks them until the process dies.
• Engine URL construction is per-engine and user input reaches it. A query must be URL-encoded before it is concatenated; an unencoded term produces wrong results at best and a malformed request at worst.
• HTML parsing is against pages we do not control. IPrsrAnchor extracts anchors from markup that changes without notice — a parser change needs a guard so a missing element degrades quietly instead of raising on every search.
• Search-engine scraping is rate-limited and blocked. A change that adds parallelism, retry storms, or user-agent rotation to evade blocking is out of scope for an archive; note it and stop.
• CheckRButton maps the radio buttons to engines. Adding an engine means a control, a mapping entry, and a URL builder — keep the mapping table shaped as a table rather than growing an if chain.
• Treat this as an archive. No modernisation, no reformatting, no new dependencies. The Delphi conventions in USS.pas are the standard.
• Clear.bat defines build output (.dcu, .opt, .dsm, …); none of it belongs in a commit.

Commands a reviewer should be able to quote

# open SS.dpr in Borland Delphi 7 and compile (Ctrl+F9)
Clear.bat

YAML

See YAML Conventions. The extension is .yaml, never .yml. String values are single-quoted; double quotes appear only where interpolation or an escape needs them; booleans and numbers are never quoted. This applies to workflows, compose files, manifests, and YAML blocks inside Markdown.

Tests

There is no test suite. Verification is compiling in Delphi 7 and running a search against each selected engine, confirming the anchor extraction and the domain and language filters still behave.

Documentation and change control

See Documentation & Change Control and CHANGELOG Formatting.

This repository uses chlog fragments. CHANGELOG.md is generated and is never edited by hand.

• Every change ships a fragment created with chlog new --kind <Kind> --body '…', staged in the same commit as the code. Kinds: Added, Changed, Deprecated, Removed, Fixed, Security.
• A backward-incompatible change to the public interface additionally carries --breaking. The kind alone never triggers a major bump.
• A hand-edited CHANGELOG.md, or a code change with no fragment under .changes/unreleased/, is a Critical finding — chlog check fails the build for it.
• Fragment bodies start with a lowercase verb in simple past tense, capitalise proper nouns (GitHub, Go, Docker), and wrap code identifiers and versions in backticks.
• README.md is updated whenever usage, setup, configuration, or architecture changes; .github/copilot-instructions.md and CLAUDE.md whenever the workflow, commands, or structure changes. Documentation and code ship in one commit.

Git Flow and pull-request hygiene

See Git Flow and Merge Guide.

• Branch names are feat/, fix/, refactor/, chore/, test/, or docs/ followed by a ticket ID or a short slug — feat/TICKET-000, fix/input-mask.
• Commit subjects are type(SCOPE): message: simple past tense (added, fixed, changed, removed), lowercase first word, no trailing period, code identifiers in backticks.
• Branches are synchronised with git rebase, never git merge. A merge commit from the default branch inside a feature branch is a finding.
• Breaking changes are flagged in three places: the commit footer (**BREAKING CHANGE:** …), the changelog, and the pull-request description. One or two of the three is not enough.
• Versions follow SemVer: MAJOR for incompatible changes, MINOR for features, PATCH for fixes.

Security

See Security.

• No hard-coded secrets. API keys, tokens, passwords, and private keys belong in environment variables or a secret manager — never in source, tests, fixtures, or the changelog. A secret that reaches a commit must be rotated, not merely deleted.
• Never write a PEM header sentinel or a realistic key shape into a fixture (GitHub ghp_ prefixes, OpenAI sk- prefixes, AWS AKIA prefixes, Slack xoxb prefixes, JWT-shaped strings, or the dashed BEGIN … banners). Gitleaks matches the shape, not the value, so a placeholder that merely looks like a credential fails the pipeline. Use inert placeholders such as fixture-token-placeholder.
• Suppressions must be justified. Entries in .gitleaksignore, .trivyignore, .semgrepignore, or .codeql-false-positives need a fingerprint, a dated comment, and a reason. A suppression added to silence a real finding is a Critical.
• Validate and sanitise every external input; use parameterised queries; apply least privilege; keep secrets out of logs.
• Dependency manifest changes are reviewed for new transitive vulnerabilities. When a fix exists, bump the version rather than suppressing the finding.

What not to flag

A review that raises noise gets ignored. Do not report these:

• Anything that amounts to "this is old code". Modernising idiom, renaming identifiers, reformatting, or introducing a framework into a preserved archive is out of scope and destroys the historical record.
• Mixed Portuguese and English identifiers and UI strings — that is the original code.
• The single ~1100-line unit — the program is one unit by design.
• The absence of tests, linting, or a CI compile step.
• Anything the guide does not require and this file does not list, unless it is a genuine correctness or security defect — say so plainly and label it a Suggestion.

Review output format

## Code review: <branch or PR>

### Critical (must fix before merge)
- `path/to/file.ext:LINE` — <what is wrong> — violates <rule> (<guide page or repo file>)

### Warning (should fix)
- `path/to/file.ext:LINE` — <what is wrong> — violates <rule>

### Suggestion (optional)
- `path/to/file.ext:LINE` — <improvement>

### Change-control checklist
- [ ] Changelog entry present for every behavioural change
- [ ] `README.md` updated if usage, setup, or architecture changed
- [ ] `.github/copilot-instructions.md` and `CLAUDE.md` updated if the workflow, commands, or structure changed
- [ ] Commit messages follow `type(SCOPE): message` in simple past tense
- [ ] Breaking changes flagged in the commit footer, the changelog, and the PR description

### Verdict: APPROVE / REQUEST CHANGES
<one paragraph: the blocking findings, or why the change is ready>

Severity

Rank findings most severe first, and state plainly when nothing blocks the merge — an empty Critical section is a valid, useful review.