feat: bootstrap production-grade starknet-skills foundation#1
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a Claude plugin manifest, comprehensive Starknet/Cairo SKILL documentation, dataset schemas and records, audit ingest/extraction and pipeline scripts, evaluation cases and scorecards, CI quality checks, and many supporting README and utility files. Changes
Sequence Diagram(s)sequenceDiagram
rect rgba(63,81,181,0.5)
participant Operator as Operator
end
rect rgba(0,150,136,0.5)
participant Fetch as fetch-and-extract.sh
end
rect rgba(255,152,0,0.5)
participant Extract as pdftotext/mutool
end
rect rgba(233,30,99,0.5)
participant Repo as Repository (datasets/raw & datasets/extracted)
end
rect rgba(156,39,176,0.5)
participant Pipeline as audit-pipeline scripts
end
Operator->>Fetch: provide URLs list
Fetch->>Extract: download PDFs and extract text
Extract->>Repo: write raw PDFs and extracted .txt
Repo->>Pipeline: pipeline reads extracted text and manifests
Pipeline->>Pipeline: segment_text.py → segments JSONL
Pipeline->>Pipeline: validate_jsonl.py and generate_manifest.py (sha256)
Pipeline->>Repo: write manifests, segments, normalized findings
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
🧹 Nitpick comments (2)
.claude-plugin/plugin.json (1)
1-6: LGTM – Valid plugin manifest with consistent metadata.The
namefield matches the SKILL.md front matter, and version0.1.0aligns with the bootstrap intent.Consider adding optional metadata fields in future iterations:
📝 Optional metadata expansion
{ "name": "starknet-skills", "version": "0.1.0", "description": "Cairo/Starknet reasoning and audit-quality skills", - "author": "keep-starknet-strange" + "author": "keep-starknet-strange", + "repository": "https://github.com/keep-starknet-strange/starknet-skills", + "license": "MIT" }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.claude-plugin/plugin.json around lines 1 - 6, No changes required: the plugin manifest fields "name" and "version" in the .claude-plugin/plugin.json are valid and consistent with SKILL.md and bootstrap intent; keep "name": "starknet-skills" and "version": "0.1.0" as-is, and optionally consider adding extra metadata fields (e.g., "license", "homepage", "repository") in a future update if desired.CONTRIBUTING.md (1)
32-36: Define the evaluator threshold explicitly.
evaluator pass thresholdis not actionable yet because the required score isn't named anywhere in this section. A concrete number or a specific scorecard reference would make the gate enforceable instead of subjective.Suggested fix
- evaluator pass threshold +- evaluator pass threshold (for example: the relevant scorecard in `evals/scorecards/` must stay at or above the documented target)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CONTRIBUTING.md` around lines 32 - 36, Replace the vague "evaluator pass threshold" bullet with an explicit, actionable threshold and a reference to the scorecard used; for example change that list item to something like "evaluator pass threshold: minimum 85% (CI evaluator scorecard v1.2) — see scorecard definition and link," or specify "score >= 8/10 on Evaluator Scorecard"; ensure the exact numeric threshold and the scorecard identifier/reference are added so the gate is enforceable and unambiguous.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.gitignore:
- Around line 5-7: Update the .gitignore to broaden environment-file ignores:
replace the single ".env" entry with a glob that ignores common variants (e.g.,
".env.*" or ".env*") so files like ".env.local" and ".env.development" are
ignored, and explicitly unignore any checked-in sample (e.g., add an allow-list
entry like "!.env.example") so the repository can still contain a sample env
file; update the entry around the existing ".venv/" / ".env" block that contains
these lines.
In `@cairo-auditor/README.md`:
- Around line 6-8: Update the README.md entry that currently reads
"`references/audit-findings/` stores normalized findings from historical
audits." to reference the canonical repository location `datasets/audits/` (or
explicitly state that `references/audit-findings/` is a local cache derived from
`datasets/audits/`); edit the README.md line (the list item about audit
findings) so it either points to `datasets/audits/` as the canonical store or
clarifies the cache relationship.
In `@cairo-auditor/references/audit-findings/source-cairo-security-import.md`:
- Line 30: Replace the incorrect authoring module name "cairo-contracts" with
the actual module added in this PR, "cairo-contract-authoring", so the line
"**Not for:** Writing contracts (use cairo-contracts), testing (use
cairo-testing), gas optimization (use cairo-optimization)" points to the real
authoring skill; update the phrase "use cairo-contracts" to "use
cairo-contract-authoring" and verify any other references to "cairo-contracts"
are corrected.
- Around line 15-18: Update the "Versions" and "Cairo Editions" sections to use
version-pinned documentation URLs and release tags instead of unstable
`latest`/`alpha`/`tmp` references: replace any links to unstable docs with the
stable Cairo, Starknet Foundry, and OpenZeppelin Cairo 3.x URLs (e.g.,
https://www.starknet.io/cairo-book/ and
https://docs.starknet.io/build/corelib/intro for Cairo,
https://foundry-rs.github.io/starknet-foundry/ for Foundry, and
https://docs.openzeppelin.com/contracts-cairo/3.x for OZ), change GitHub release
references to explicit tags like `v2.15.0` (avoid `latest` and `-alpha` tags),
and if you must keep non-pinned context add a clear note such as "examples
validated as of [date/version]" to the "Versions" or "Cairo Editions"
paragraphs.
In `@cairo-auditor/references/checklists/release-gate.md`:
- Around line 3-6: Add an explicit held-out evaluation gate to the release
checklist: insert a new bullet like "Held-out evals/scorecard pass for relevant
cases (no regressions beyond allowed threshold)" alongside the existing items
(e.g., near "Static analysis and test suites pass" or after "Findings
disposition table updated") and reference the scorecard or test set name(s) used
for validation so the release requires passing those eval cases before merging.
In `@cairo-auditor/SKILL.md`:
- Around line 58-60: The relative path in SKILL.md points to ../../evals/cases/,
which climbs above the repo root; update the reference string to ../evals/cases/
so it correctly points to the sibling evals directory (replace the
../../evals/cases/ token in SKILL.md with ../evals/cases/).
- Around line 47-55: The SKILL.md checklist is missing fields required by the
validator; update the "Each finding must include:" list in SKILL.md to exactly
match the contract enforced by scripts/audit-extraction/validate-finding.sh by
adding the required keys: finding_id, source_audit, file_path, function_name,
fixed_pattern, detection_rule, false_positive_caveat, provenance, and confidence
(alongside the existing severity, root cause, exploit path, fix recommendation,
and required regression test) and ensure naming/formatting matches the validator
expectations so contributor output will validate.
In `@cairo-contract-authoring/SKILL.md`:
- Around line 355-367: The fenced code block in SKILL.md lacks a language tag
which triggers markdownlint; update the opening fence for the project tree block
from ``` to ```text so the snippet (the my-project/ tree block) is annotated and
the linter stops flagging it.
In `@cairo-testing/SKILL.md`:
- Around line 260-279: The example should not early-return or reference the
nonexistent assume; instead define a custom Fuzzable type (e.g., struct
BoundedAmount with field value: u256) and implement the Fuzzable trait for it so
the fuzzer generates only valid values via generate_arg(1, 1000000); then change
the test signature to accept amount: BoundedAmount and call
dispatcher.transfer(USER(), amount.value). Locate and update the examples around
the test_transfer_bounded function, the BoundedAmount type, and its Fuzzable
implementation (use generate_arg in generate()/generate_arg()) to constrain the
input rather than filtering inside the test.
In `@cairo-toolchain/SKILL.md`:
- Around line 47-50: The fenced code block listing tool versions (the
triple-backtick block containing "scarb 2.15.1" and "starknet-foundry 0.56.0")
lacks a language tag and triggers markdownlint; update that fenced block to
include the text language tag (i.e., change ``` to ```text) so the block becomes
a ```text fenced code block, preserving the existing lines and spacing.
- Around line 38-40: Remove the insecure "curl -L ... | sh" installer line and
its associated one-liner `snfoundryup` from SKILL.md so the section no longer
contradicts the reproducibility guidance; either replace that snippet with
instructions to download a specific tagged GitHub release asset and verify its
SHA256 checksum, or delete the alternative entirely and point readers to the
existing `asdf` recommendation and the repository's `.tool-versions` mention for
pinned installs.
In `@evals/README.md`:
- Around line 10-15: The "Minimum Gate" policy is ambiguous because it
references an undefined "allowed threshold"; update the Minimum Gate section to
specify the exact comparison baseline (e.g., "compared to the current main
branch model" or "vs. baseline release vX.Y.Z") and state a concrete numeric
false-positive threshold (e.g., "false-positive rate must not increase by more
than 1.0 percentage point or must remain below 2% absolute"). Edit the Minimum
Gate text to replace "allowed threshold" with the chosen baseline identifier and
the chosen numeric threshold, and include a brief note on the measurement method
(metric name and dataset) so reviewers can apply the gate consistently.
In `@scripts/audit-extraction/fetch-and-extract.sh`:
- Around line 49-52: The current mutool branch always returns 0 even if mutool
draw fails; update the if block that calls mutool draw (the command using mutool
draw -F txt -o "$txt_path" "$pdf_path") to check mutool's exit status and
propagate it (e.g., run mutool draw and if it succeeds return 0, otherwise
return its non‑zero exit code or a non‑zero value). In short: replace the
unconditional return 0 with logic that tests the mutool command result and
returns that result so failures are not masked.
In `@scripts/audit-extraction/normalize-template.md`:
- Around line 7-19: The template's output fields are inconsistent with the audit
schema: add the required field source_audit to the Output fields list and split
the combined file/function into two distinct fields named file_path and
function_name (ensure any code that constructs records from this template uses
these exact keys). Update the Output fields section in normalize-template.md to
include source_audit, file_path, and function_name (removing or renaming
file/function), and adjust any mappings or validators that reference file_path
or function_name so generated records match datasets/audits/schema.md exactly.
In `@scripts/audit-extraction/validate-finding.sh`:
- Around line 14-15: The script currently proceeds into the per-field loop even
when the input FILE is unreadable or contains invalid JSON, causing misleading
"Missing required field: finding_id" messages; before the per-field loop that
checks fields like "finding_id" add an explicit readability check for FILE
(ensure it exists and is readable) and then validate the JSON content is
parseable and is a JSON object/array (e.g., use jq or a json parser) and bail
with a clear error like "Unreadable file: $FILE" or "Invalid JSON in $FILE:
<parse error>" if those checks fail; only after those validations run the
existing per-field loop to check required keys.
---
Nitpick comments:
In @.claude-plugin/plugin.json:
- Around line 1-6: No changes required: the plugin manifest fields "name" and
"version" in the .claude-plugin/plugin.json are valid and consistent with
SKILL.md and bootstrap intent; keep "name": "starknet-skills" and "version":
"0.1.0" as-is, and optionally consider adding extra metadata fields (e.g.,
"license", "homepage", "repository") in a future update if desired.
In `@CONTRIBUTING.md`:
- Around line 32-36: Replace the vague "evaluator pass threshold" bullet with an
explicit, actionable threshold and a reference to the scorecard used; for
example change that list item to something like "evaluator pass threshold:
minimum 85% (CI evaluator scorecard v1.2) — see scorecard definition and link,"
or specify "score >= 8/10 on Evaluator Scorecard"; ensure the exact numeric
threshold and the scorecard identifier/reference are added so the gate is
enforceable and unambiguous.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f7b64e6d-acb0-447f-b8e0-319ed5220951
📒 Files selected for processing (49)
.claude-plugin/plugin.json.gitignoreCLAUDE.mdCONTRIBUTING.mdREADME.mdSKILL.mdaccount-abstraction/SKILL.mdaccount-abstraction/references/README.mdcairo-auditor/README.mdcairo-auditor/SKILL.mdcairo-auditor/VERSIONcairo-auditor/agents/adversarial.mdcairo-auditor/agents/vector-scan.mdcairo-auditor/references/audit-findings/README.mdcairo-auditor/references/audit-findings/source-cairo-security-import.mdcairo-auditor/references/checklists/release-gate.mdcairo-auditor/references/vulnerability-db/AA-SELF-CALL-SESSION.mdcairo-auditor/references/vulnerability-db/README.mdcairo-auditor/scripts/README.mdcairo-auditor/workflows/deep.mdcairo-auditor/workflows/default.mdcairo-contract-authoring/SKILL.mdcairo-contract-authoring/references/README.mdcairo-optimization/SKILL.mdcairo-optimization/references/README.mdcairo-testing/SKILL.mdcairo-testing/references/README.mdcairo-toolchain/SKILL.mdcairo-toolchain/references/README.mddatasets/audits/README.mddatasets/audits/examples/finding-template.jsondatasets/audits/extracted/.gitkeepdatasets/audits/raw/.gitkeepdatasets/audits/schema.mdevals/README.mdevals/cases/.gitkeepevals/cases/case-aa-self-call-session.jsonevals/scorecards/.gitkeepevals/scorecards/v0.1.0-baseline.mdllms.txtopenzeppelin-cairo/SKILL.mdopenzeppelin-cairo/references/README.mdscripts/audit-extraction/README.mdscripts/audit-extraction/fetch-and-extract.shscripts/audit-extraction/normalize-template.mdscripts/audit-extraction/urls.example.txtscripts/audit-extraction/validate-finding.shstarknet-network-facts/SKILL.mdstarknet-network-facts/references/README.md
Contributor
Author
|
@greptileai review |
Greptile SummaryThis PR bootstraps the Key issues identified:
Confidence Score: 3/5
|
| Filename | Overview |
|---|---|
| scripts/audit-pipeline/generate_manifest.py | Generates audits.jsonl from a seed JSON; includes path-traversal protection via relative_to, HTTPS enforcement on source_url, and held-out policy checks. Non-deterministic ingested_at timestamp on every run creates spurious diffs in committed artifacts. |
| scripts/audit-pipeline/check_no_heldout_leak.py | Scans dataset artifacts for blocked held-out audit IDs; handles JSON/JSONL parsing errors correctly. Missing coverage of evals/cases/ directory — a future case derived from a held-out audit would not be detected. |
| cairo-auditor/SKILL.md | Defines the cairo-auditor skill contract; includes vectorized scanning workflow and reporting fields. fixed_snippet and test_that_catches_it are listed as mandatory in the Reporting Contract but are optional/nullable in finding.schema.json, creating an enforcement gap. |
| scripts/audit-pipeline/validate_jsonl.py | JSONL validator using Draft202012Validator; handles malformed JSON, blocked IDs, and format checking. FormatChecker() may silently skip uri format validation without the rfc3987 package (not installed in CI). |
| scripts/audit-pipeline/segment_text.py | Segments extracted audit text by headings; uses errors="replace" for UTF-8, per-audit watermark scoping, and broadened DETAIL_MARKERS. The ID_HEADING regex uses \d{2,} (not \d{2}) so triple-digit finding numbers are handled correctly. |
| scripts/audit-extraction/fetch-and-extract.sh | Downloads and extracts audit PDFs; enforces HTTPS-only scheme, collision-safe hash-prefixed filenames, and PDF magic-byte validation. Error message when pdftotext is installed but fails is misleading (says neither tool is available). |
| .github/workflows/quality.yml | CI quality gate with pinned action SHAs; runs ruff, shellcheck, JSONL validation, skill contract checks, and held-out leak detection. pip install jsonschema pyyaml has no version pin, making CI non-deterministic across future runs. |
| datasets/normalized/finding.schema.json | JSON Schema for normalized findings; uses additionalProperties: false, correct required array, and appropriate enums. fixed_snippet and test_that_catches_it are optional nullable, inconsistent with cairo-auditor/SKILL.md reporting contract. |
| datasets/normalized/audit.schema.json | JSON Schema for normalized audit metadata; additionalProperties: false. source_url has no scheme or format constraint, unlike audit-manifest.schema.json which enforces "format": "uri". |
| datasets/manifests/audit_metadata.seed.json | Seed metadata for the two ingested audits; raw_path values use bare filenames without hash prefixes, which will not match the fetch-and-extract.sh-generated collision-safe filenames (e.g., <hash>_Vesu_Update_Audit_Report.pdf), causing generate_manifest.py to throw FileNotFoundError for operators following the documented pipeline. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[urls.example.txt\nURL list] --> B[fetch-and-extract.sh\nDownload + PDF validation]
B --> C[datasets/audits/raw/\nhash_filename.pdf]
B --> D[datasets/audits/extracted/\nhash_filename.txt]
E[audit_metadata.seed.json\nSeed metadata] --> F[generate_manifest.py\nHash + path validation]
C --> F
D --> F
F --> G[datasets/manifests/audits.jsonl\nManifest with SHA-256]
D --> H[segment_text.py\nHeading-based segmentation]
H --> I[datasets/segments/*.jsonl\nSegmented findings]
J[Manual extraction] --> K[datasets/normalized/findings/*.jsonl\nNormalized findings JSONL]
J --> L[datasets/normalized/audits/*.json\nNormalized audit metadata]
G --> M{validate_jsonl.py\nSchema + blocked ID check}
K --> M
I --> N{check_no_heldout_leak.py\nHeld-out audit ID scan}
G --> N
K --> N
L --> N
M --> O[CI Quality Gate\n.github/workflows/quality.yml]
N --> O
K --> P[cairo-auditor SKILL\nVectorized scan workflow]
P --> Q[evals/cases/*.json\nEval test cases]
Q --> R[evals/scorecards/\nBaseline scorecard]
Last reviewed commit: 49254b6
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
11 tasks
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (3)
scripts/audit-extraction/validate-finding.sh (1)
14-15:⚠️ Potential issue | 🟡 MinorFail fast on unreadable or malformed input.
Right now a missing file or invalid JSON falls through to the per-field loop and gets reported as
Missing required field: ..., which hides the actual failure mode.🛠️ Proposed fix
FILE="$1" + +if [ ! -r "$FILE" ]; then + echo "Cannot read file: $FILE" >&2 + exit 1 +fi + +jq -e 'type == "object"' "$FILE" >/dev/null 2>&1 || { + echo "Invalid JSON object: $FILE" >&2 + exit 1 +} required=(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/audit-extraction/validate-finding.sh` around lines 14 - 15, The script should fail fast on unreadable/malformed input: check that the input variable FILE is provided and points to a readable file (test FILE exists and is readable) before entering the per-field loop, then validate that the file contains well-formed JSON (e.g., run a json parser/check step) and if either check fails, print a clear error including the FILE name and the parser error and exit non-zero; update the validation flow so the per-field loop only runs after these checks succeed.cairo-contract-authoring/SKILL.md (1)
362-374:⚠️ Potential issue | 🟡 MinorSame lint issue here: annotate the project-tree fence.
This block still has a bare fence, so
markdownlintwill keep reporting MD040 until it is tagged, e.g. withtext.Suggested fix
-``` +```text my-project/ Scarb.toml src/ lib.cairo # mod declarations contract.cairo # main contract interfaces.cairo # trait definitions components/ mod.cairo my_component.cairo tests/ test_contract.cairo</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@cairo-contract-authoring/SKILL.mdaround lines 362 - 374, The code block in
SKILL.md containing the project tree is missing a language/info string which
triggers markdownlint MD040; update the fenced block around the tree (the
triple-backtick block showing "my-project/ ... test_contract.cairo") to include
an info string (e.g., text) so the fence reads ```text and keep the same
content—this will silence MD040 while preserving the example.</details> </blockquote></details> <details> <summary>cairo-testing/SKILL.md (1)</summary><blockquote> `269-285`: _⚠️ Potential issue_ | _🟡 Minor_ **Don't document `assume`/early-return as the bounded-fuzzing pattern for `snforge`.** This is still open from the earlier review. The current Starknet Foundry docs describe fuzzing via test parameters/custom `Fuzzable` types and enumerate the supported test attributes, and `assume` is not among them. As written, this snippet just discards generated cases via early return, so it teaches a weaker pattern than the tool actually supports. ([foundry-rs.github.io](https://foundry-rs.github.io/starknet-foundry/snforge-advanced-features/fuzz-testing.html?utm_source=openai)) <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@cairo-testing/SKILL.md` around lines 269 - 285, The example in test_transfer_bounded shows an unsupported/weak pattern (early-return/"assume") for snforge fuzzing; remove or replace this snippet and update the text to document the supported bounded-fuzzing approach: explain using test parameters with custom Fuzzable types and the official #[fuzzer(...)] attributes instead of discarding cases, and provide a correct example that defines a Fuzzable type (or uses the documented attribute-based bounds) and uses that type in the test function signature (replace references to test_transfer_bounded and its current #[fuzzer] usage accordingly). ``` </details> </blockquote></details> </blockquote></details> <details> <summary>🧹 Nitpick comments (9)</summary><blockquote> <details> <summary>datasets/distilled/vuln-cards/SYSCALL_SELECTOR_FALLBACK_ASSUMPTION.md (1)</summary><blockquote> `38-38`: **Consider hyphenating "onchain" for consistency.** The term "onchain" could be spelled "on-chain" with a hyphen for consistency with traditional style guides. While both forms are used in the blockchain space, the hyphenated form is more conventional in formal documentation. This applies to both "onchain" and "offchain" in this line. <details> <summary>✍️ Optional style adjustment</summary> ```diff -- Offchain simulation helper code outside onchain execution. +- Off-chain simulation helper code outside on-chain execution. ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@datasets/distilled/vuln-cards/SYSCALL_SELECTOR_FALLBACK_ASSUMPTION.md` at line 38, Update the phrase "Offchain simulation helper code outside onchain execution." to use hyphenated forms for consistency: change "Offchain" to "Off-chain" and "onchain" to "on-chain" (i.e., update the sentence in the document text where that exact phrase appears). ``` </details> </blockquote></details> <details> <summary>scripts/audit-extraction/validate-finding.sh (1)</summary><blockquote> `16-41`: **Keep the finding contract in one place.** The script's hard-coded `required` array currently mirrors `datasets/normalized/finding.schema.json`, but duplicating this list creates a maintenance burden. If the schema changes, developers must remember to update both locations. Consider reading the schema's `required` array dynamically instead of hard-coding it. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@scripts/audit-extraction/validate-finding.sh` around lines 16 - 41, The hard-coded required=(...) array in validate-finding.sh duplicates datasets/normalized/finding.schema.json; replace that literal by reading the schema's required array at runtime (e.g., use jq to extract .required into a bash array and assign it to the variable required) and use that array elsewhere unchanged; update the script to fail with a clear error if the schema file is missing or jq is unavailable, and keep the symbol name required so existing checks (in validate-finding.sh) continue to work without further changes. ``` </details> </blockquote></details> <details> <summary>scripts/audit-pipeline/validate_jsonl.py (1)</summary><blockquote> `17-27`: **Derive enum checks from the schema instead of hardcoding them here.** Right now the validator duplicates `severity_normalized` and `confidence` locally but ignores any other schema enums. That will drift the first time the schema changes, and it means the script is only partially validating what the loaded schema already knows. <details> <summary>Suggested refactor</summary> ```diff -def validate_record(rec: dict, required: list[str], allowed: set[str]) -> list[str]: +def validate_record( + rec: dict, + required: list[str], + properties: dict[str, dict], + allowed: set[str], +) -> list[str]: errors = [] for key in required: if key not in rec: errors.append(f"missing required key: {key}") elif rec[key] in (None, "", []): errors.append(f"empty required key: {key}") - if "severity_normalized" in rec and rec.get("severity_normalized") not in { - "critical", - "high", - "medium", - "low", - "info", - "best_practice", - }: - errors.append(f"invalid severity_normalized: {rec.get('severity_normalized')}") - if "confidence" in rec and rec.get("confidence") not in {"low", "medium", "high"}: - errors.append(f"invalid confidence: {rec.get('confidence')}") + for key, value in rec.items(): + enum_values = properties.get(key, {}).get("enum") + if enum_values is not None and value not in enum_values: + errors.append(f"invalid {key}: {value}") unknown = set(rec.keys()) - allowed if unknown: errors.append("unknown keys: " + ", ".join(sorted(unknown))) return errors @@ schema = json.loads(Path(args.schema).read_text()) required = schema.get("required", []) - allowed = set(schema.get("properties", {}).keys()) + properties = schema.get("properties", {}) + allowed = set(properties.keys()) @@ - errs = validate_record(rec, required, allowed) + errs = validate_record(rec, required, properties, allowed) ``` </details> Also applies to: 40-42 <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@scripts/audit-pipeline/validate_jsonl.py` around lines 17 - 27, Replace the hardcoded allowed-value sets with enums pulled from the loaded JSON schema: for each field checked (e.g., severity_normalized and confidence) read the enum from the schema properties (e.g., schema["properties"]["severity_normalized"]["enum"] and schema["properties"]["confidence"]["enum"]) and use that set to validate rec.get("severity_normalized") and rec.get("confidence"), appending to errors on mismatch; fall back to the existing literal sets only if the schema enum is missing, and apply the same pattern to the similar checks around lines 40-42 to avoid duplicating allowed values. ``` </details> </blockquote></details> <details> <summary>datasets/manifests/audit-manifest.schema.json (1)</summary><blockquote> `25-25`: **Consider adding `format: "uri"` to `repo_url`.** The `source_url` field has `"format": "uri"` validation, but `repo_url` is typed as plain string. For consistency, consider adding URI format validation to `repo_url` as well. <details> <summary>📝 Proposed fix</summary> ```diff - "repo_url": { "type": "string" }, + "repo_url": { "type": "string", "format": "uri" }, ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@datasets/manifests/audit-manifest.schema.json` at line 25, The repo_url property is currently defined only as a string; add JSON Schema URI validation by adding "format": "uri" to the "repo_url" definition so it matches the existing "source_url" validation. Update the "repo_url" schema entry (the property named "repo_url") to include the format key and ensure downstream consumers/tests that rely on URI validation for "source_url" now also accept/validate "repo_url" accordingly. ``` </details> </blockquote></details> <details> <summary>scripts/audit-pipeline/segment_text.py (2)</summary><blockquote> `49-59`: **Clarify `end_page` semantics when flushing segments.** When a new heading is detected, the previous segment's `end_page` is set to the current page (where the new heading appears). This means a segment starting on page 3 that ends when a new heading appears on page 5 will have `end_page=5`. Depending on intent, this could be considered an off-by-one: - If `end_page` should be exclusive or represent the last page containing segment content, consider `end_page: current_page - 1` or `page_idx - 1`. - If inclusive semantics are intended (segment may span into the same page as the next heading), the current logic is correct. Consider adding a brief comment clarifying the intended semantics. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@scripts/audit-pipeline/segment_text.py` around lines 49 - 59, The code sets the previous segment's end_page to current_page (when a new heading is detected), which may be off-by-one depending on whether end_page is intended to be inclusive or exclusive; update the flush logic in the block that appends to segments (referencing variables current, current.lines, end_page, current_page, page_idx and the Seg constructor) to either set "end_page": page_idx - 1 for exclusive/last-content-page semantics or leave it as page_idx for inclusive semantics, and add a concise inline comment above the append explaining the chosen semantics; also run/update any tests or consumers that assume a particular end_page interpretation. ``` </details> --- `117-117`: **Consider logging a warning when decode errors occur.** Using `errors="ignore"` silently drops bytes that can't be decoded as UTF-8. For PDF-extracted text, this is a pragmatic choice, but consider logging a warning so operators know if significant data loss occurred. <details> <summary>📝 Optional: Add a warning for decode issues</summary> ```python # Alternative approach: use errors="replace" and log if replacement chars appear text = in_path.read_text(encoding="utf-8", errors="replace") if "\ufffd" in text: import sys print(f"Warning: {args.input} contains undecodable bytes", file=sys.stderr) ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@scripts/audit-pipeline/segment_text.py` at line 117, The current call to in_path.read_text(..., errors="ignore") silently drops undecodable bytes; change to use errors="replace" (or capture exceptions) and then detect the Unicode replacement character "\ufffd" in the resulting text and emit a warning via the script's logger (or logging.warning) including the filename (referencing in_path and the segment_text caller) so operators are alerted if decode issues occurred; update the line that builds segments (segments = segment_text(...)) to read the text into a variable first, check for "\ufffd", log the warning, then pass the text into segment_text. ``` </details> </blockquote></details> <details> <summary>datasets/normalized/finding.schema.json (2)</summary><blockquote> `43-44`: **Consider adding `minItems: 0` explicitly or documenting empty-array semantics.** The `contracts` and `functions` arrays lack `minItems` constraints (unlike `tags` which has `minItems: 1`). This implicitly allows empty arrays. If at least one contract should always be specified, add `minItems: 1`. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@datasets/normalized/finding.schema.json` around lines 43 - 44, The schema entries for arrays "contracts" and "functions" currently allow empty arrays; update their JSON Schema definitions to explicitly include a minItems constraint (e.g., add "minItems": 1 to each if at least one item is required, or "minItems": 0 if empty arrays are allowed) and, if semantics require, add a brief description explaining empty-array behavior to match the existing "tags" pattern that uses "minItems": 1; modify the "contracts" and "functions" properties in the schema accordingly. ``` </details> --- `5-30`: **Consider relaxing some required fields for flexibility.** All 29 fields are required, which may be overly strict. Some fields like `fixed_snippet`, `test_that_catches_it`, or `false_positive_lookalikes` may not always be available (e.g., for unfixed findings or findings without clear test strategies). Consider making some fields optional or allowing empty strings/arrays where data isn't available. Fields that are commonly missing in practice: - `fixed_snippet` (unfixed findings) - `test_that_catches_it` (not all findings have clear test approaches) - `false_positive_lookalikes` (may be empty) <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@datasets/normalized/finding.schema.json` around lines 5 - 30, The schema's required array is too strict—remove "fixed_snippet", "test_that_catches_it", and "false_positive_lookalikes" from the "required" list and update their individual definitions so they accept empty values: for "fixed_snippet" and "test_that_catches_it" allow type ["string","null"] or set "nullable": true and permit empty strings, and for "false_positive_lookalikes" allow an empty array by setting "type": "array" with "minItems": 0 (or allow ["array","null"]) so records without these fields validate. ``` </details> </blockquote></details> <details> <summary>datasets/manifests/audit_metadata.seed.json (1)</summary><blockquote> `1-26`: **Document the seed file relationship to the final manifest schema.** The `audit-manifest.schema.json` requires `raw_sha256` and `extracted_sha256` fields, but `audit_metadata.seed.json` intentionally omits them because the `generate_manifest.py` pipeline computes these hashes before generating the final manifest. This is sound design, but it creates confusion. Either create a separate JSON schema for seed files or explicitly document in the README that seed files are pre-pipeline inputs and do not need to comply with the full manifest schema. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@datasets/manifests/audit_metadata.seed.json` around lines 1 - 26, The seed file audit_metadata.seed.json omits raw_sha256 and extracted_sha256 because generate_manifest.py computes them before producing the final manifest; update docs or schema to avoid confusion by either (A) adding a new lightweight schema (e.g., audit-seed.schema.json) that matches audit_metadata.seed.json, or (B) adding an explicit note to the README explaining that audit_metadata.seed.json is a pre-pipeline input and is not required to include raw_sha256/extracted_sha256 and that generate_manifest.py produces the final audit-manifest.json conforming to audit-manifest.schema.json; reference the files audit_metadata.seed.json, audit-manifest.schema.json and the pipeline script generate_manifest.py in the change so reviewers can see the mapping. ``` </details> </blockquote></details> </blockquote></details> <details> <summary>🤖 Prompt for all review comments with AI agents</summary>Verify each finding against the current code and only fix it if needed.
Inline comments:
In@cairo-testing/SKILL.md:
- Around line 136-139: The negative-path example in SKILL.md currently
impersonates USER() with start_cheat_caller_address(contract_address, USER())
but never calls the protected function, so it doesn't assert the owner check;
either remove the failing-path text or convert the snippet into a proper failing
test by adding an unauthorized call such as dispatcher.owner_only_function() and
marking the example as a panic test (e.g., use a #[should_panic] example or
equivalent) so the unauthorized access is actually exercised and fails as
intended.- Around line 351-357: In the SKILL.md documentation code fence that lists the
test files (the block that begins with "tests/" and lists test_unit.cairo,
test_integration.cairo, test_fuzz.cairo, helpers.cairo), annotate the opening
fence with a language tag (e.g., change the leadingtotext) so the code
block is tagged and resolves the markdownlint MD040 warning.In
@datasets/normalized/audit.schema.json:
- Around line 16-21: The schema currently allows empty strings for required
metadata because JSON Schema's "required" only ensures keys exist; update the
properties for audit_id, project, auditor, source_url, and repository to
disallow empty values by adding "minLength": 1 to each of those string property
definitions in audit.schema.json (keep the existing "date" format unchanged);
this ensures records with "" for audit_id, project, auditor, source_url, or
repository will fail validation.In
@datasets/normalized/findings/csc_vesu_update_2025_03.findings.jsonl:
- Line 2: The finding CSC-VESU-002 has conflicting fields: "status" is "fixed"
while "notes" state "Report status unresolved but client update states fixed in
commit"; update either the status or the notes to remove ambiguity—for example,
change the notes to explicitly state the resolution and reference the commit
that fixed it (e.g., "Fixed in commit ; report previously unresolved") or,
if your schema supports it, set status to a more precise value like
"fixed_post_report" and leave notes explaining the timeline; ensure you update
the JSON fields "status" and/or "notes" for finding_id "CSC-VESU-002"
accordingly so downstream consumers see a single clear final resolution.In
@datasets/segments/erim_nostra_pools_2024_01.jsonl:
- Around line 2-8: The JSONL segments contain watermark/layout artifacts (e.g.,
tokens like "V.", "Er", "im", "Rev", "ev", "ie", "w", "R") and broken
multi-column fragments inside the content fields (see heading_key values L-02,
I-01, I-02, I-03, BP-01, BP-02, BP-03 and their segment_id values) — add a
cleanup pass that strips known watermark/footer tokens, collapses stray
single-letter fragments, reorders or joins multi-column text into proper
sentence flow, and re-generate the records so each content field contains
contiguous, human-readable sentences without watermark noise before committing.In
@evals/heldout/README.md:
- Around line 11-12: Ensure held-out records are excluded from the entire
datasets pipeline (ingest, segment, normalize, distill) rather than only
datasets/distilled/: update any filtering logic that references held-out ids to
run at the earliest stage (ingest) and persist the exclusion through segment and
normalize steps, add checks in functions or scripts that produce
datasets/segments/ and datasets/normalized/* to refuse writing held-out
records, and add regression tests that assert no held-out ids appear in
datasets/segments/, datasets/normalized/ or datasets/distilled/* so leakage
cannot reoccur.In
@scripts/audit-pipeline/generate_manifest.py:
- Around line 35-40: The current checks on raw_path and extracted_path don't
prevent absolute paths or '..' from escaping repo_root; resolve both repo_root
and each candidate path (use Path.resolve(strict=False)) and verify each
resolved candidate is under the resolved repo_root (e.g., with
Path.is_relative_to or by checking the repo_root is a parent) before calling
exists() or opening them; if a path escapes, raise FileNotFoundError or a
security-specific exception for that seed row referencing
raw_path/extracted_path so poisoned rows cannot read files outside repo_root.In
@scripts/audit-pipeline/validate_jsonl.py:
- Around line 45-49: Wrap the json.loads(line) call in a try/except that catches
json.JSONDecodeError and records a line-level error instead of raising (e.g.,
set errs to a list containing a "Malformed JSON" message with the exception
text), then continue handling that line; additionally, after parsing, check that
rec is a dict (not a list/number/null) and if it's not, set errs to include a
"expected JSON object" error before calling validate_record, or skip calling
validate_record entirely for non-dict values so the validator reports the line
error rather than crashing; reference the loop that iterates over
Path(args.jsonl).read_text().splitlines(), the json.loads(line) call, the rec
variable, and validate_record(rec, required, allowed).
Duplicate comments:
In@cairo-contract-authoring/SKILL.md:
- Around line 362-374: The code block in SKILL.md containing the project tree is
missing a language/info string which triggers markdownlint MD040; update the
fenced block around the tree (the triple-backtick block showing "my-project/ ...
test_contract.cairo") to include an info string (e.g., text) so the fence readsexample. In `@cairo-testing/SKILL.md`: - Around line 269-285: The example in test_transfer_bounded shows an unsupported/weak pattern (early-return/"assume") for snforge fuzzing; remove or replace this snippet and update the text to document the supported bounded-fuzzing approach: explain using test parameters with custom Fuzzable types and the official #[fuzzer(...)] attributes instead of discarding cases, and provide a correct example that defines a Fuzzable type (or uses the documented attribute-based bounds) and uses that type in the test function signature (replace references to test_transfer_bounded and its current #[fuzzer] usage accordingly). In `@scripts/audit-extraction/validate-finding.sh`: - Around line 14-15: The script should fail fast on unreadable/malformed input: check that the input variable FILE is provided and points to a readable file (test FILE exists and is readable) before entering the per-field loop, then validate that the file contains well-formed JSON (e.g., run a json parser/check step) and if either check fails, print a clear error including the FILE name and the parser error and exit non-zero; update the validation flow so the per-field loop only runs after these checks succeed. --- Nitpick comments: In `@datasets/distilled/vuln-cards/SYSCALL_SELECTOR_FALLBACK_ASSUMPTION.md`: - Line 38: Update the phrase "Offchain simulation helper code outside onchain execution." to use hyphenated forms for consistency: change "Offchain" to "Off-chain" and "onchain" to "on-chain" (i.e., update the sentence in the document text where that exact phrase appears). In `@datasets/manifests/audit_metadata.seed.json`: - Around line 1-26: The seed file audit_metadata.seed.json omits raw_sha256 and extracted_sha256 because generate_manifest.py computes them before producing the final manifest; update docs or schema to avoid confusion by either (A) adding a new lightweight schema (e.g., audit-seed.schema.json) that matches audit_metadata.seed.json, or (B) adding an explicit note to the README explaining that audit_metadata.seed.json is a pre-pipeline input and is not required to include raw_sha256/extracted_sha256 and that generate_manifest.py produces the final audit-manifest.json conforming to audit-manifest.schema.json; reference the files audit_metadata.seed.json, audit-manifest.schema.json and the pipeline script generate_manifest.py in the change so reviewers can see the mapping. In `@datasets/manifests/audit-manifest.schema.json`: - Line 25: The repo_url property is currently defined only as a string; add JSON Schema URI validation by adding "format": "uri" to the "repo_url" definition so it matches the existing "source_url" validation. Update the "repo_url" schema entry (the property named "repo_url") to include the format key and ensure downstream consumers/tests that rely on URI validation for "source_url" now also accept/validate "repo_url" accordingly. In `@datasets/normalized/finding.schema.json`: - Around line 43-44: The schema entries for arrays "contracts" and "functions" currently allow empty arrays; update their JSON Schema definitions to explicitly include a minItems constraint (e.g., add "minItems": 1 to each if at least one item is required, or "minItems": 0 if empty arrays are allowed) and, if semantics require, add a brief description explaining empty-array behavior to match the existing "tags" pattern that uses "minItems": 1; modify the "contracts" and "functions" properties in the schema accordingly. - Around line 5-30: The schema's required array is too strict—remove "fixed_snippet", "test_that_catches_it", and "false_positive_lookalikes" from the "required" list and update their individual definitions so they accept empty values: for "fixed_snippet" and "test_that_catches_it" allow type ["string","null"] or set "nullable": true and permit empty strings, and for "false_positive_lookalikes" allow an empty array by setting "type": "array" with "minItems": 0 (or allow ["array","null"]) so records without these fields validate. In `@scripts/audit-extraction/validate-finding.sh`: - Around line 16-41: The hard-coded required=(...) array in validate-finding.sh duplicates datasets/normalized/finding.schema.json; replace that literal by reading the schema's required array at runtime (e.g., use jq to extract .required into a bash array and assign it to the variable required) and use that array elsewhere unchanged; update the script to fail with a clear error if the schema file is missing or jq is unavailable, and keep the symbol name required so existing checks (in validate-finding.sh) continue to work without further changes. In `@scripts/audit-pipeline/segment_text.py`: - Around line 49-59: The code sets the previous segment's end_page to current_page (when a new heading is detected), which may be off-by-one depending on whether end_page is intended to be inclusive or exclusive; update the flush logic in the block that appends to segments (referencing variables current, current.lines, end_page, current_page, page_idx and the Seg constructor) to either set "end_page": page_idx - 1 for exclusive/last-content-page semantics or leave it as page_idx for inclusive semantics, and add a concise inline comment above the append explaining the chosen semantics; also run/update any tests or consumers that assume a particular end_page interpretation. - Line 117: The current call to in_path.read_text(..., errors="ignore") silently drops undecodable bytes; change to use errors="replace" (or capture exceptions) and then detect the Unicode replacement character "\ufffd" in the resulting text and emit a warning via the script's logger (or logging.warning) including the filename (referencing in_path and the segment_text caller) so operators are alerted if decode issues occurred; update the line that builds segments (segments = segment_text(...)) to read the text into a variable first, check for "\ufffd", log the warning, then pass the text into segment_text. In `@scripts/audit-pipeline/validate_jsonl.py`: - Around line 17-27: Replace the hardcoded allowed-value sets with enums pulled from the loaded JSON schema: for each field checked (e.g., severity_normalized and confidence) read the enum from the schema properties (e.g., schema["properties"]["severity_normalized"]["enum"] and schema["properties"]["confidence"]["enum"]) and use that set to validate rec.get("severity_normalized") and rec.get("confidence"), appending to errors on mismatch; fall back to the existing literal sets only if the schema enum is missing, and apply the same pattern to the similar checks around lines 40-42 to avoid duplicating allowed values.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID:
3d3fd25b-361d-4a82-8c06-d33a120034aa📒 Files selected for processing (46)
README.mdcairo-auditor/SKILL.mdcairo-auditor/references/audit-findings/README.mdcairo-contract-authoring/SKILL.mdcairo-testing/SKILL.mddatasets/README.mddatasets/audits/README.mddatasets/audits/examples/finding-template.jsondatasets/audits/schema.mddatasets/distilled/README.mddatasets/distilled/fix-patterns/FIX_MODE_PRECEDENCE.mddatasets/distilled/fix-patterns/INPUT_BOUND_VALIDATION.mddatasets/distilled/fix-patterns/README.mddatasets/distilled/fix-patterns/REMOVE_DEAD_FALLBACKS.mddatasets/distilled/test-recipes/README.mddatasets/distilled/test-recipes/TEST_FEE_BOUNDARY.mddatasets/distilled/test-recipes/TEST_SELECTOR_FALLBACK_REMOVAL.mddatasets/distilled/test-recipes/TEST_SHUTDOWN_PRECEDENCE.mddatasets/distilled/vuln-cards/README.mddatasets/distilled/vuln-cards/SHUTDOWN_OVERRIDE_PRECEDENCE.mddatasets/distilled/vuln-cards/SYSCALL_SELECTOR_FALLBACK_ASSUMPTION.mddatasets/distilled/vuln-cards/UNCHECKED_FEE_BOUND.mddatasets/manifests/README.mddatasets/manifests/audit-manifest.schema.jsondatasets/manifests/audit_metadata.seed.jsondatasets/manifests/audits.jsonldatasets/normalized/README.mddatasets/normalized/audit.schema.jsondatasets/normalized/audits/csc_vesu_update_2025_03.jsondatasets/normalized/audits/erim_nostra_pools_2024_01.jsondatasets/normalized/finding.schema.jsondatasets/normalized/findings/csc_vesu_update_2025_03.findings.jsonldatasets/normalized/findings/erim_nostra_pools_2024_01.findings.jsonldatasets/segments/README.mddatasets/segments/csc_vesu_update_2025_03.jsonldatasets/segments/erim_nostra_pools_2024_01.jsonlevals/README.mdevals/heldout/README.mdevals/scorecards/v0.1.1-audit-pipeline.mdscripts/audit-extraction/README.mdscripts/audit-extraction/normalize-template.mdscripts/audit-extraction/validate-finding.shscripts/audit-pipeline/README.mdscripts/audit-pipeline/generate_manifest.pyscripts/audit-pipeline/segment_text.pyscripts/audit-pipeline/validate_jsonl.py✅ Files skipped from review due to trivial changes (15)
- datasets/distilled/vuln-cards/README.md
- datasets/distilled/fix-patterns/README.md
- scripts/audit-pipeline/README.md
- datasets/segments/README.md
- evals/scorecards/v0.1.1-audit-pipeline.md
- datasets/distilled/test-recipes/TEST_SHUTDOWN_PRECEDENCE.md
- datasets/manifests/audits.jsonl
- datasets/normalized/README.md
- datasets/README.md
- datasets/segments/csc_vesu_update_2025_03.jsonl
- datasets/distilled/vuln-cards/SHUTDOWN_OVERRIDE_PRECEDENCE.md
- datasets/normalized/findings/erim_nostra_pools_2024_01.findings.jsonl
- datasets/distilled/test-recipes/README.md
- datasets/distilled/fix-patterns/FIX_MODE_PRECEDENCE.md
- datasets/distilled/fix-patterns/REMOVE_DEAD_FALLBACKS.md
🚧 Files skipped from review as they are similar to previous changes (6)
- cairo-auditor/references/audit-findings/README.md
- datasets/audits/README.md
- evals/README.md
- scripts/audit-extraction/README.md
- scripts/audit-extraction/normalize-template.md
- cairo-auditor/SKILL.md
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (2)
cairo-auditor/README.md (1)
7-8:⚠️ Potential issue | 🟠 MajorAlign the canonical findings path with the repo-wide dataset location.
This still looks inconsistent with the PR’s documented audit corpus layout. The README now says the canonical store is
datasets/normalized/findings/, but the PR objectives describedatasets/audits/as the repository location added for normalized audit findings. Please point this section at the actual canonical path, or explicitly explain howdatasets/normalized/findings/relates todatasets/audits/.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cairo-auditor/README.md` around lines 7 - 8, Update the README.md to correct or clarify the canonical findings path: either change the listed canonical store from `datasets/normalized/findings/` to `datasets/audits/` to match the PR objective, or add a short sentence explaining how `datasets/normalized/findings/` maps to or differs from `datasets/audits/` (e.g., one is the normalized output location vs. the repo-wide audits corpus), and ensure `references/audit-findings/` remains marked as legacy/imported material.cairo-testing/SKILL.md (1)
136-139:⚠️ Potential issue | 🟡 MinorThe negative-path example is incomplete and doesn't demonstrate the failure.
After impersonating
USER(), the snippet ends with a comment but never callsdispatcher.owner_only_function(). This teaches an incorrect pattern: the test will always pass because no protected function is invoked.Either remove the failing-path prose or convert this into a proper
#[should_panic]test that actually calls the protected function.✅ Suggested fix: Proper failing test
- // Impersonate USER — should fail - start_cheat_caller_address(contract_address, USER()); - // This should panic +} + +#[test] +#[should_panic(expected: 'Caller is not the owner')] +fn test_only_owner_fails() { + let contract_address = deploy_contract(); + let dispatcher = IMyContractDispatcher { contract_address }; + + start_cheat_caller_address(contract_address, USER()); + dispatcher.owner_only_function(); // should panic }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cairo-testing/SKILL.md` around lines 136 - 139, The negative-path test currently impersonates USER() with start_cheat_caller_address(contract_address, USER()) but never invokes the protected call, so it cannot fail; update the test to actually call the protected function (e.g., dispatcher.owner_only_function()) while marked as a failing test by adding the #[should_panic] attribute (or remove the failing-path prose). Locate the test that uses start_cheat_caller_address and either add the call to dispatcher.owner_only_function() and annotate the test with #[should_panic] or delete the incomplete negative-path example so it no longer suggests a failing scenario without invoking the protected method.
🧹 Nitpick comments (3)
datasets/normalized/finding.schema.json (1)
43-54: Prevent duplicate values in normalized arrays.For a normalized dataset, repeated
contracts,functions,tags, orsource_pagesshould not validate. Right now one record can carry duplicates and still pass, which will skew downstream counts and faceting.Proposed hardening
- "contracts": { "type": "array", "items": { "type": "string" } }, - "functions": { "type": "array", "items": { "type": "string" } }, + "contracts": { "type": "array", "items": { "type": "string" }, "uniqueItems": true }, + "functions": { "type": "array", "items": { "type": "string" }, "uniqueItems": true }, @@ - "false_positive_lookalikes": { "type": "array", "items": { "type": "string" } }, - "tags": { "type": "array", "items": { "type": "string" }, "minItems": 1 }, - "source_pages": { "type": "array", "items": { "type": "integer", "minimum": 1 }, "minItems": 1 }, + "false_positive_lookalikes": { "type": "array", "items": { "type": "string" }, "uniqueItems": true }, + "tags": { "type": "array", "items": { "type": "string" }, "minItems": 1, "uniqueItems": true }, + "source_pages": { "type": "array", "items": { "type": "integer", "minimum": 1 }, "minItems": 1, "uniqueItems": true },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@datasets/normalized/finding.schema.json` around lines 43 - 54, The schema allows duplicate entries in arrays like "contracts", "functions", "tags", and "source_pages", causing skewed counts; update the JSON schema in finding.schema.json by adding "uniqueItems": true to each of those array definitions (preserving existing "items", "minItems" and "minimum" constraints) so duplicates will fail validation and downstream faceting/counts remain accurate.cairo-toolchain/SKILL.md (1)
208-219: Consider more robust output parsing withsncast --jsonflag.The script uses
grepandawkto extract values fromsncastoutput, which can break if output formatting changes.sncastsupports a global--jsonflag (placed right aftersncastand before the subcommand) that provides structured output:TOKEN_CLASS=$(sncast --json declare --contract-name MyToken --url $RPC_URL --account $ACCOUNT | jq -r '.class_hash')This approach is more resilient to output format changes compared to text parsing.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cairo-toolchain/SKILL.md` around lines 208 - 219, Replace fragile grep/awk parsing of sncast output by invoking sncast with the global --json flag (sncast --json ...) for the declare and deploy calls and parse the JSON with jq; specifically update the commands that set TOKEN_CLASS, TOKEN_ADDR, AMM_CLASS, and AMM_ADDR to call sncast --json declare and sncast --json deploy and extract .class_hash for declarations and .contract_address for deployments using jq -r, keeping existing constructor-calldata values unchanged.cairo-auditor/references/audit-findings/source-cairo-security-import.md (1)
52-52: Clarify terminology for broader audience.The phrase "griefs the sequencer" uses gaming slang that may not be clear to all readers. Consider rephrasing to "burdens the sequencer" or "degrades sequencer performance" for improved clarity in a professional security document.
📝 Suggested rewording
-9. **`__validate__` in custom accounts must be lightweight.** No storage writes (except nonce), no external calls, bounded gas. Expensive validation griefs the sequencer. (Section 12) +9. **`__validate__` in custom accounts must be lightweight.** No storage writes (except nonce), no external calls, bounded gas. Expensive validation burdens the sequencer. (Section 12)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cairo-auditor/references/audit-findings/source-cairo-security-import.md` at line 52, The wording "griefs the sequencer" in the rule describing __validate__ for custom accounts is informal; update the sentence to use clearer professional phrasing (e.g., "burdens the sequencer" or "degrades sequencer performance") so the guidance is understandable to a broader audience—modify the text around the "__validate__ in custom accounts must be lightweight" line to replace "griefs the sequencer" with one of these clearer alternatives.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cairo-toolchain/SKILL.md`:
- Line 48: Replace the ambiguous term "snforge 0.56.0" in the SKILL.md note with
the consistent project name used elsewhere—e.g., "starknet-foundry (snforge)
0.56.0" or simply "starknet-foundry 0.56.0"—so readers see the canonical tool
name; locate the string "snforge 0.56.0" in the note and update it accordingly
while preserving the link to the release page.
- Line 259: Update the note text to accurately state that sncast verify supports
both Walnut and Voyager verification backends (as of Starknet Foundry v0.56.0+),
and mention the CLI option to use Voyager by example: "sncast verify --verifier
voyager"; replace the incorrect sentence that says only Walnut is supported and
add a short parenthetical or version note referencing v0.56.0+.
In `@datasets/normalized/finding.schema.json`:
- Around line 20-22: The schema currently lists fixed_snippet and
test_that_catches_it as required properties (also duplicated in the other
required group at the later block), which forces placeholder values for
unresolved or third‑party findings; update the JSON Schema by removing
"fixed_snippet" and "test_that_catches_it" from the required arrays (and any
duplicate required group around the later block) so those fields are optional,
leaving "recommendation" or other core fields as required if appropriate; locate
the required arrays that mention fixed_snippet/test_that_catches_it and delete
those entries so records can omit remediation artifacts.
- Around line 32-58: The string properties currently only enforce type presence
which allows empty "" values; add a non-empty constraint (e.g., "minLength": 1)
to each required string field such as "finding_id", "project", "root_cause",
"recommendation", "notes" (and other core string props you want non-empty like
"auditor", "severity_original", "status") so validation rejects empty
strings—update each property's schema entry (e.g., the "finding_id", "project",
"root_cause", "recommendation", "notes" property objects) to include
"minLength": 1 (or an equivalent pattern) while leaving existing enum/format
rules intact.
- Line 36: The schema's "date" property declares "format": "date" but the custom
validator in scripts/audit-pipeline/validate_jsonl.py does not check format, so
either make the validator enforce JSON Schema formats (e.g., replace or augment
the current lightweight validator with a proper library like jsonschema and use
FormatChecker in validate_jsonl.py when validating the finding.schema.json's
"date" field), or remove the "format": "date" declaration from
finding.schema.json to avoid a misleading constraint; update validate_jsonl.py
to use jsonschema.validate(..., format_checker=FormatChecker()) if you choose
the former and ensure the "date" property in finding.schema.json remains as is.
In `@scripts/audit-pipeline/generate_manifest.py`:
- Around line 35-53: The manifest currently re-emits the original seed strings
(rec = dict(row)) instead of the validated/resolved paths; update the code after
computing raw_path and extracted_path to canonicalize the emitted fields by
replacing rec["raw_path"] and rec["extracted_path"] with their repo-relative,
normalized string forms derived from raw_path and extracted_path (e.g.,
raw_path.relative_to(repo_root_resolved) /
extracted_path.relative_to(repo_root_resolved) converted to a stable string form
such as as_posix()), leaving the other computed fields (raw_sha256,
extracted_sha256, ingested_at) unchanged so the generated list uses canonical,
non-escaping, machine-independent paths.
---
Duplicate comments:
In `@cairo-auditor/README.md`:
- Around line 7-8: Update the README.md to correct or clarify the canonical
findings path: either change the listed canonical store from
`datasets/normalized/findings/` to `datasets/audits/` to match the PR objective,
or add a short sentence explaining how `datasets/normalized/findings/` maps to
or differs from `datasets/audits/` (e.g., one is the normalized output location
vs. the repo-wide audits corpus), and ensure `references/audit-findings/`
remains marked as legacy/imported material.
In `@cairo-testing/SKILL.md`:
- Around line 136-139: The negative-path test currently impersonates USER() with
start_cheat_caller_address(contract_address, USER()) but never invokes the
protected call, so it cannot fail; update the test to actually call the
protected function (e.g., dispatcher.owner_only_function()) while marked as a
failing test by adding the #[should_panic] attribute (or remove the failing-path
prose). Locate the test that uses start_cheat_caller_address and either add the
call to dispatcher.owner_only_function() and annotate the test with
#[should_panic] or delete the incomplete negative-path example so it no longer
suggests a failing scenario without invoking the protected method.
---
Nitpick comments:
In `@cairo-auditor/references/audit-findings/source-cairo-security-import.md`:
- Line 52: The wording "griefs the sequencer" in the rule describing
__validate__ for custom accounts is informal; update the sentence to use clearer
professional phrasing (e.g., "burdens the sequencer" or "degrades sequencer
performance") so the guidance is understandable to a broader audience—modify the
text around the "__validate__ in custom accounts must be lightweight" line to
replace "griefs the sequencer" with one of these clearer alternatives.
In `@cairo-toolchain/SKILL.md`:
- Around line 208-219: Replace fragile grep/awk parsing of sncast output by
invoking sncast with the global --json flag (sncast --json ...) for the declare
and deploy calls and parse the JSON with jq; specifically update the commands
that set TOKEN_CLASS, TOKEN_ADDR, AMM_CLASS, and AMM_ADDR to call sncast --json
declare and sncast --json deploy and extract .class_hash for declarations and
.contract_address for deployments using jq -r, keeping existing
constructor-calldata values unchanged.
In `@datasets/normalized/finding.schema.json`:
- Around line 43-54: The schema allows duplicate entries in arrays like
"contracts", "functions", "tags", and "source_pages", causing skewed counts;
update the JSON schema in finding.schema.json by adding "uniqueItems": true to
each of those array definitions (preserving existing "items", "minItems" and
"minimum" constraints) so duplicates will fail validation and downstream
faceting/counts remain accurate.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4a615abe-8573-4ef8-bc98-2fc263051dcc
📒 Files selected for processing (15)
.gitignorecairo-auditor/README.mdcairo-auditor/SKILL.mdcairo-auditor/references/audit-findings/source-cairo-security-import.mdcairo-auditor/references/checklists/release-gate.mdcairo-contract-authoring/SKILL.mdcairo-testing/SKILL.mdcairo-toolchain/SKILL.mddatasets/normalized/finding.schema.jsonevals/README.mdevals/cases/case-aa-self-call-session.jsonscripts/audit-extraction/fetch-and-extract.shscripts/audit-extraction/normalize-template.mdscripts/audit-extraction/validate-finding.shscripts/audit-pipeline/generate_manifest.py
✅ Files skipped from review due to trivial changes (2)
- cairo-auditor/references/checklists/release-gate.md
- evals/README.md
🚧 Files skipped from review as they are similar to previous changes (7)
- .gitignore
- scripts/audit-extraction/normalize-template.md
- evals/cases/case-aa-self-call-session.json
- scripts/audit-extraction/validate-finding.sh
- cairo-contract-authoring/SKILL.md
- cairo-auditor/SKILL.md
- scripts/audit-extraction/fetch-and-extract.sh
Contributor
Author
|
@greptileai review |
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/quality.yml:
- Around line 36-49: The workflow currently hardcodes specific files in the
"Validate findings JSONL" and "Validate manifest JSONL" steps causing new
findings/manifests to bypass schema checks; replace the explicit python calls
with directory iteration that runs scripts/audit-pipeline/validate_jsonl.py for
every matching file (e.g., iterate datasets/normalized/findings/*.jsonl using
the finding.schema.json and iterate datasets/manifests/*.jsonl and
datasets/normalized/audits/*.json using the audit-manifest.schema.json) so all
files are validated automatically; update the steps named "Validate findings
JSONL" and "Validate manifest JSONL" to use a shell loop or find+xargs to invoke
validate_jsonl.py for each file rather than listing files individually.
In `@cairo-testing/SKILL.md`:
- Around line 169-185: The Block Number and Sequencer Address examples use
undefined identifiers; wrap each in a proper #[test] function, mirroring the
surrounding examples: call deploy_contract() to set contract_address, then call
start_cheat_block_number(contract_address, 500) /
stop_cheat_block_number(contract_address) inside that test, and similarly set
sequencer via contract_address_const and call
start_cheat_sequencer_address(contract_address, sequencer) /
stop_cheat_sequencer_address(contract_address) in a separate #[test]; ensure
variable names and calls match start_cheat_block_number,
stop_cheat_block_number, start_cheat_sequencer_address,
stop_cheat_sequencer_address, deploy_contract(), and contract_address_const.
- Around line 381-395: The helpers module is missing required snforge_std
imports used by declare(...) and its result helpers; in tests/helpers.cairo add
an import line that brings in declare plus the traits needed to call
.contract_class() and .unwrap() (e.g., declare, ContractClassTrait,
DeclareResultTrait) at the top of the file so deploy_my_contract() can compile
and use declare("MyContract").unwrap().contract_class().
- Around line 312-324: The example is missing the import for
contract_address_const used in the test; add the line importing
contract_address_const from the starknet crate at the top of the snippet (so the
test fn test_against_mainnet can call contract_address_const::<...>()), keeping
the existing use snforge_std::BlockTag and existing IERC20Dispatcher usage so
the example compiles when copied.
In `@datasets/normalized/audit.schema.json`:
- Line 22: The "scope_files" property allows empty-string items; update its item
schema (the "scope_files" array definition) to disallow blank entries by adding
"minLength": 1 to the items object (i.e., change items from { "type": "string" }
to { "type": "string", "minLength": 1 }) so validation rejects "" values while
preserving the array type and semantics.
In `@datasets/normalized/findings/csc_vesu_update_2025_03.findings.jsonl`:
- Line 2: The functions array contains an invalid descriptive entry "shutdown
mode extension path" which is not a valid Cairo function identifier and may
break downstream tooling; update the finding by replacing that string with the
actual function name (e.g., the snake_case Cairo symbol that implements the
shutdown overwrite behavior) or remove the entry if it refers to a code path
rather than a function, ensuring the functions array only contains valid
identifiers like "set_shutdown_mode_overwrite" or other real function symbols
referenced in the Ekubo extension.
In `@scripts/audit-pipeline/check_no_heldout_leak.py`:
- Around line 64-72: The current loop only iterates four hard-coded
subdirectories and misses any future or nested datasets; replace the fixed
directory lists with a recursive scan under repo_root / "datasets" using rglob
to find all "*.json" and "*.jsonl" files, then call check_json_file(path,
blocked) for ".json" and check_jsonl_file(path, blocked) for ".jsonl"
(preserving the existing issues list and use of check_jsonl_file/check_json_file
and the repo_root variable) so any new subtree like datasets/distilled/ will be
covered.
In `@scripts/audit-pipeline/generate_manifest.py`:
- Around line 63-71: validate_seed_rows currently only checks for non-empty
strings and an https prefix; extend it to fully enforce manifest schema by (1)
validating row["date"] is a valid ISO 8601 date/time (use datetime.fromisoformat
or a strict parser and raise ValueError on failure), (2) validating
row["source_url"] is a well-formed HTTPS URL (use urllib.parse.urlparse and
ensure scheme == "https" and netloc is non-empty), and (3) validating
row["source_type"] is one of the allowed values (check against the known
set/list of allowed source types and raise ValueError if not). Keep error
messages informative and reference the row index (idx) like the existing checks
so invalid seed rows are rejected immediately.
In `@scripts/audit-pipeline/segment_text.py`:
- Around line 63-85: The flush uses current_page which can have advanced beyond
the page that actually contributed the last line; instead, track the last page
that contributed content for the active segment (e.g., add a last_content_page
field on Seg or a local variable updated whenever you set or append lines) and
use that value for "end_page" when appending to segments; update
last_content_page both when you initialize current (start_page) and whenever you
append a line to current.lines so the two append sites (the block creating a new
segment and the final flush) emit end_page=last_content_page rather than
current_page.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8f3c565a-45b7-4f12-a2c3-d554943a85b9
📒 Files selected for processing (17)
.github/workflows/quality.ymlcairo-testing/SKILL.mdcairo-toolchain/SKILL.mddatasets/manifests/audit-manifest.schema.jsondatasets/normalized/audit.schema.jsondatasets/normalized/finding.schema.jsondatasets/normalized/findings/csc_vesu_update_2025_03.findings.jsonldatasets/segments/erim_nostra_pools_2024_01.jsonlevals/heldout/README.mdevals/heldout/audit_ids.txtscripts/audit-extraction/fetch-and-extract.shscripts/audit-extraction/validate-finding.shscripts/audit-pipeline/README.mdscripts/audit-pipeline/check_no_heldout_leak.pyscripts/audit-pipeline/generate_manifest.pyscripts/audit-pipeline/segment_text.pyscripts/audit-pipeline/validate_jsonl.py
✅ Files skipped from review due to trivial changes (1)
- evals/heldout/audit_ids.txt
🚧 Files skipped from review as they are similar to previous changes (6)
- scripts/audit-extraction/validate-finding.sh
- datasets/normalized/finding.schema.json
- scripts/audit-extraction/fetch-and-extract.sh
- datasets/segments/erim_nostra_pools_2024_01.jsonl
- cairo-toolchain/SKILL.md
- scripts/audit-pipeline/validate_jsonl.py
Comment on lines
+169
to
+185
| ```cairo | ||
| use snforge_std::{start_cheat_block_number, stop_cheat_block_number}; | ||
|
|
||
| start_cheat_block_number(contract_address, 500); | ||
| // ... test logic | ||
| stop_cheat_block_number(contract_address); | ||
| ``` | ||
|
|
||
| ### Sequencer Address | ||
|
|
||
| ```cairo | ||
| use snforge_std::{start_cheat_sequencer_address, stop_cheat_sequencer_address}; | ||
|
|
||
| start_cheat_sequencer_address(contract_address, sequencer); | ||
| // ... test logic | ||
| stop_cheat_sequencer_address(contract_address); | ||
| ``` |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check if the file exists and how many lines it has
wc -l cairo-testing/SKILL.mdRepository: keep-starknet-strange/starknet-skills
Length of output: 107
🏁 Script executed:
# Read the file around lines 169-185 with context
sed -n '150,200p' cairo-testing/SKILL.mdRepository: keep-starknet-strange/starknet-skills
Length of output: 1357
🏁 Script executed:
# Also check the entire file structure to understand the pattern
head -200 cairo-testing/SKILL.md | tail -100Repository: keep-starknet-strange/starknet-skills
Length of output: 2763
Wrap Block Number and Sequencer Address cheatcode examples in runnable tests.
The Block Number and Sequencer Address sections (lines 169-184) contain free-floating statements with undefined identifiers (contract_address and sequencer). This makes them non-executable and inconsistent with the surrounding Caller Address, Block Timestamp, and Expected Panic examples, which all use proper #[test] functions with complete context. Update both snippets to include a #[test] function that defines contract_address via deploy_contract() and sequencer via contract_address_const, mirroring the pattern used in the adjacent sections.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cairo-testing/SKILL.md` around lines 169 - 185, The Block Number and
Sequencer Address examples use undefined identifiers; wrap each in a proper
#[test] function, mirroring the surrounding examples: call deploy_contract() to
set contract_address, then call start_cheat_block_number(contract_address, 500)
/ stop_cheat_block_number(contract_address) inside that test, and similarly set
sequencer via contract_address_const and call
start_cheat_sequencer_address(contract_address, sequencer) /
stop_cheat_sequencer_address(contract_address) in a separate #[test]; ensure
variable names and calls match start_cheat_block_number,
stop_cheat_block_number, start_cheat_sequencer_address,
stop_cheat_sequencer_address, deploy_contract(), and contract_address_const.
Comment on lines
+312
to
+324
| ```cairo | ||
| use snforge_std::BlockTag; | ||
|
|
||
| #[test] | ||
| #[fork(url: "https://starknet-mainnet.g.alchemy.com/v2/YOUR_KEY", block_tag: latest)] | ||
| fn test_against_mainnet() { | ||
| let usdc_address = contract_address_const::<0x053c91253bc9682c04929ca02ed00b3e423f6710d2ee7e0d5ebb06f3ecf368a8>(); | ||
| let dispatcher = IERC20Dispatcher { contract_address: usdc_address }; | ||
|
|
||
| let supply = dispatcher.total_supply(); | ||
| assert(supply > 0, 'USDC should have supply'); | ||
| } | ||
| ``` |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd "SKILL.md" --type fRepository: keep-starknet-strange/starknet-skills
Length of output: 311
🏁 Script executed:
wc -l cairo-testing/SKILL.mdRepository: keep-starknet-strange/starknet-skills
Length of output: 107
🏁 Script executed:
sed -n '310,330p' cairo-testing/SKILL.mdRepository: keep-starknet-strange/starknet-skills
Length of output: 708
🏁 Script executed:
sed -n '300,330p' cairo-testing/SKILL.mdRepository: keep-starknet-strange/starknet-skills
Length of output: 950
🌐 Web query:
contract_address_const Cairo Starknet import path
💡 Result:
In Cairo 1 (Starknet corelib), contract_address_const lives in the starknet::contract_address module, so you can import it as:
use starknet::contract_address::contract_address_const;It’s also commonly available via the starknet prelude-style import:
use starknet::{ContractAddress, contract_address_const};(Underlying fully-qualified module: core::starknet::contract_address.) [1], [2]
🏁 Script executed:
rg "contract_address_const" cairo-testing/SKILL.md -B 5 -A 5Repository: keep-starknet-strange/starknet-skills
Length of output: 1470
Add the missing contract_address_const import to the fork example.
This snippet uses contract_address_const::<...>() but never imports it, so the example is incomplete when copied on its own. Other examples in this file correctly import it from starknet::contract_address_const.
Suggested fix
```cairo
use snforge_std::BlockTag;
+use starknet::contract_address_const;
#[test]
#[fork(url: "https://starknet-mainnet.g.alchemy.com/v2/YOUR_KEY", block_tag: latest)]
fn test_against_mainnet() {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cairo-testing/SKILL.md` around lines 312 - 324, The example is missing the
import for contract_address_const used in the test; add the line importing
contract_address_const from the starknet crate at the top of the snippet (so the
test fn test_against_mainnet can call contract_address_const::<...>()), keeping
the existing use snforge_std::BlockTag and existing IERC20Dispatcher usage so
the example compiles when copied.
Comment on lines
+381
to
+395
| ```cairo | ||
| // tests/helpers.cairo | ||
| use starknet::ContractAddress; | ||
| use starknet::contract_address_const; | ||
|
|
||
| fn OWNER() -> ContractAddress { contract_address_const::<'OWNER'>() } | ||
| fn USER() -> ContractAddress { contract_address_const::<'USER'>() } | ||
| fn ZERO() -> ContractAddress { contract_address_const::<0>() } | ||
|
|
||
| fn deploy_my_contract() -> ContractAddress { | ||
| let contract = declare("MyContract").unwrap().contract_class(); | ||
| let (addr, _) = contract.deploy(@array![OWNER().into()]).unwrap(); | ||
| addr | ||
| } | ||
| ``` |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "SKILL.md" -type fRepository: keep-starknet-strange/starknet-skills
Length of output: 329
🏁 Script executed:
sed -n '381,395p' ./cairo-testing/SKILL.mdRepository: keep-starknet-strange/starknet-skills
Length of output: 585
🏁 Script executed:
rg -t cairo "declare\(" --max-count 20 -B 3 | head -100Repository: keep-starknet-strange/starknet-skills
Length of output: 110
🏁 Script executed:
fd -e cairo -type f | head -20Repository: keep-starknet-strange/starknet-skills
Length of output: 251
🏁 Script executed:
fd -e cairo | head -20Repository: keep-starknet-strange/starknet-skills
Length of output: 63
🏁 Script executed:
rg "declare" ./cairo-testing/SKILL.md -B 5 -A 5Repository: keep-starknet-strange/starknet-skills
Length of output: 1445
🏁 Script executed:
sed -n '360,395p' ./cairo-testing/SKILL.mdRepository: keep-starknet-strange/starknet-skills
Length of output: 1089
Add missing snforge_std imports to the helpers module snippet.
The tests/helpers.cairo snippet uses declare("MyContract").unwrap().contract_class() but omits the required snforge_std imports. Since this is presented as a standalone file (indicated by the section title and filename comment), it should include all necessary imports for the code to compile.
Suggested fix
```cairo
// tests/helpers.cairo
+use snforge_std::{declare, ContractClassTrait, DeclareResultTrait};
use starknet::ContractAddress;
use starknet::contract_address_const;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cairo-testing/SKILL.md` around lines 381 - 395, The helpers module is missing
required snforge_std imports used by declare(...) and its result helpers; in
tests/helpers.cairo add an import line that brings in declare plus the traits
needed to call .contract_class() and .unwrap() (e.g., declare,
ContractClassTrait, DeclareResultTrait) at the top of the file so
deploy_my_contract() can compile and use
declare("MyContract").unwrap().contract_class().
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
cairo-testing/references/legacy-full.md (1)
33-35: Dropcasm = truefrom the default setup example.The Starknet Foundry
Scarb.tomlreference explicitly calls outcasm = trueas unnecessary overhead forsnforge/sncastunless another tool specifically needs CASM output, so keeping it in the default snippet nudges readers toward slower test builds. (foundry-rs.github.io)Suggested doc fix
[[target.starknet-contract]] sierra = true -casm = true🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cairo-testing/references/legacy-full.md` around lines 33 - 35, The example Scarb.toml snippet includes an unnecessary "casm = true" entry under the [[target.starknet-contract]] block; remove the "casm = true" line from that default example so the documentation no longer recommends enabling CASM by default (leave the [[target.starknet-contract]] block and "sierra = true" intact).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cairo-contract-authoring/references/legacy-full.md`:
- Line 5: Update the relative links in the markdown so they correctly point from
the nested references/ directory to the repo root; specifically change
occurrences like "../cairo-optimization/" to "../../cairo-optimization/" (and
similarly adjust any other "../..." link targets in this file, including the
links noted around lines 23-24) so they resolve to sibling modules and root
datasets rather than under cairo-contract-authoring.
In `@cairo-optimization/workflows/default.md`:
- Around line 16-17: Update the workflow note to require an exact reproducible
command and toolchain: state that contributors must either use the repo's pinned
toolchain via .tool-versions or explicitly list their Scarb and Foundry versions
(repo pins Scarb 2.15.1 and Starknet Foundry 0.56.0), and add the exact
resource-report invocation (e.g., run the build with "scarb run --release"
followed by the resource-report command) and the specific fields to include in
the PR (execution steps, per-step gas/compute, total gas, instruction counts,
wall-clock time, and the toolchain versions). Ensure this instruction is added
where the current “Record before/after metrics” note appears so contributors
link the optimization class used and include the full command and required
output fields.
- Line 9: Update the workflow guidance so the "Optimize storage packing"
recommendation is only applied when layout compatibility is guaranteed: add an
explicit check or note to exclude deployed or upgradeable contracts (or require
an explicit storage migration) before suggesting packing changes; reference
storage layout concerns (e.g., selector!("variable_name"), type offsets,
on-chain encoding) and document that storage-packing optimization must be
skipped for contracts with existing on-chain state or when upgradeability is
enabled, or else require a documented migration strategy.
In `@cairo-testing/references/legacy-full.md`:
- Around line 20-23: Update the broken relative dataset paths in
cairo-testing/references/legacy-full.md so they point to the repo-root datasets
directory instead of cairo-testing/datasets; replace
"../datasets/distilled/test-recipes/" with
"../../datasets/distilled/test-recipes/" and "../datasets/normalized/findings/"
with "../../datasets/normalized/findings/" so the links resolve correctly from
the references directory.
- Around line 52-59: Update the docs to clarify that the --exact flag requires a
fully-qualified test path (package::module::test_name) rather than a bare
function name; change the example using "snforge test --exact
test_erc20_transfer" to show a full qualified name like "snforge test --exact
package_name::module::test_erc20_transfer" and add a brief note next to the
--exact description contrasting it with --filter/positional patterns which
accept substrings or glob-like matches.
In `@SECURITY.md`:
- Around line 7-12: Update the "private channel" guidance in the SECURITY.md
text to name a concrete reporting channel and contact process: replace the vague
"private channel" with the actual email address or URL (e.g.,
security@yourdomain.com or a vendor triage portal), include optional PGP/GPG
public key fingerprint or instructions for encrypted submissions, and specify
expected response timeframe and any required metadata (affected module/file,
exploitability, reproduction steps, suggested remediation) to ensure reporters
have a clear, secure way to submit findings.
---
Nitpick comments:
In `@cairo-testing/references/legacy-full.md`:
- Around line 33-35: The example Scarb.toml snippet includes an unnecessary
"casm = true" entry under the [[target.starknet-contract]] block; remove the
"casm = true" line from that default example so the documentation no longer
recommends enabling CASM by default (leave the [[target.starknet-contract]]
block and "sierra = true" intact).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1c100e53-f2d1-4028-9d52-cfd3a57c374c
📒 Files selected for processing (26)
.gitignoreAGENTS.mdCODE_OF_CONDUCT.mdCONTRIBUTING.mdLICENSEREADME.mdSECURITY.mdaccount-abstraction/SKILL.mdcairo-contract-authoring/SKILL.mdcairo-contract-authoring/references/README.mdcairo-contract-authoring/references/legacy-full.mdcairo-contract-authoring/workflows/default.mdcairo-optimization/SKILL.mdcairo-optimization/references/README.mdcairo-optimization/references/legacy-full.mdcairo-optimization/workflows/default.mdcairo-testing/SKILL.mdcairo-testing/references/README.mdcairo-testing/references/legacy-full.mdcairo-testing/workflows/default.mdcairo-toolchain/SKILL.mdcairo-toolchain/references/README.mdcairo-toolchain/references/legacy-full.mdcairo-toolchain/workflows/default.mdopenzeppelin-cairo/SKILL.mdstarknet-network-facts/SKILL.md
✅ Files skipped from review due to trivial changes (5)
- LICENSE
- CODE_OF_CONDUCT.md
- cairo-optimization/references/legacy-full.md
- cairo-contract-authoring/workflows/default.md
- cairo-toolchain/workflows/default.md
🚧 Files skipped from review as they are similar to previous changes (10)
- cairo-testing/references/README.md
- cairo-toolchain/SKILL.md
- starknet-network-facts/SKILL.md
- cairo-contract-authoring/SKILL.md
- openzeppelin-cairo/SKILL.md
- cairo-contract-authoring/references/README.md
- account-abstraction/SKILL.md
- CONTRIBUTING.md
- cairo-optimization/SKILL.md
- .gitignore
Comment on lines
+52
to
+59
| # Filter to a single test function (exact match) | ||
| snforge test --exact test_erc20_transfer | ||
|
|
||
| # Run with gas reporting | ||
| snforge test --detailed-resources | ||
| ``` | ||
|
|
||
| > **Tip:** Use `snforge test --filter <pattern>` or `snforge test <pattern>` to run a subset of tests during development. `--exact` matches the full test name when you need precision. |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find files related to snforge or test command documentation
fd -t f "snforge\|test.*\.md\|cli\|command" --max-depth 3Repository: keep-starknet-strange/starknet-skills
Length of output: 63
🏁 Script executed:
# Search for documentation about the --exact flag
rg "--exact" -A 3 -B 3Repository: keep-starknet-strange/starknet-skills
Length of output: 490
🏁 Script executed:
# Look for snforge test command help or documentation
rg "snforge test" -t md -A 5Repository: keep-starknet-strange/starknet-skills
Length of output: 5069
🌐 Web query:
snforge test --exact flag behavior fully qualified test name
💡 Result:
snforge test [TEST_FILTER] normally treats TEST_FILTER as a substring match against each test’s absolute module-tree path (i.e., a fully qualified path starting from the crate/package root, with :: separators). [1][2]
With -e/--exact, TEST_FILTER must be the entire fully qualified test name, and snforge will run only the test whose name exactly equals that string (not just “contains” it). The docs explicitly note you must pass a whole qualified name like package_name::my_test, not just my_test. [1]
So, to use --exact, pass the test’s full path (package/crate + any modules + the test fn name), e.g.:
snforge test my_pkg::tests::math::adds_two --exact(shape/example)- not:
snforge test adds_two --exact[1]
Sources: [1] snforge test docs (Starknet Foundry Book), [2] Cairo book on absolute paths in the module tree.
🌐 Web query:
starknet-foundry snforge test --exact documentation
💡 Result:
In Starknet Foundry (snforge), the -e/--exact flag makes the test filter match only a test whose name exactly equals the filter string (not a substring match). The filter must be the fully-qualified test name, e.g. package_name::my_test (not just my_test). [1]
Example:
snforge test my_package::tests::test_transfer --exactSources: snforge test CLI docs (see -e, --exact). [1]
--exact requires a fully qualified test name, not a bare function name.
The --exact flag matches only the complete qualified test path (e.g., package_name::module::test_name), not a substring. The example snforge test --exact test_erc20_transfer should use the full path instead.
Suggested doc fix
-# Filter to a single test function (exact match)
-snforge test --exact test_erc20_transfer
+# Filter to a single test function (exact match)
+snforge test --exact my_package::tests::test_erc20_transfer📝 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.
Suggested change
| # Filter to a single test function (exact match) | |
| snforge test --exact test_erc20_transfer | |
| # Run with gas reporting | |
| snforge test --detailed-resources | |
| ``` | |
| > **Tip:** Use `snforge test --filter <pattern>` or `snforge test <pattern>` to run a subset of tests during development. `--exact` matches the full test name when you need precision. | |
| # Filter to a single test function (exact match) | |
| snforge test --exact my_package::tests::test_erc20_transfer | |
| # Run with gas reporting | |
| snforge test --detailed-resources |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cairo-testing/references/legacy-full.md` around lines 52 - 59, Update the
docs to clarify that the --exact flag requires a fully-qualified test path
(package::module::test_name) rather than a bare function name; change the
example using "snforge test --exact test_erc20_transfer" to show a full
qualified name like "snforge test --exact
package_name::module::test_erc20_transfer" and add a brief note next to the
--exact description contrasting it with --filter/positional patterns which
accept substrings or glob-like matches.
|
Too many files changed for review. ( |
This was referenced Mar 8, 2026
Merged
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Bootstraps
starknet-skillswith a production-grade v0.1 structure and seeds useful Cairo skill modules imported/adapted fromstarknet-agentic.What this PR adds
SKILL.md,README.md,CONTRIBUTING.md,CLAUDE.md,llms.txtskills/wrapper)cairo-auditorworkflow scaffold withreferences/,workflows/,agents/, andscripts/starknet-agentic:cairo-contract-authoringcairo-testingcairo-optimizationcairo-toolchainaccount-abstractionstarknet-network-factsopenzeppelin-cairodatasets/audits/schema + example templateevals/cases + baseline scorecardscripts/audit-extraction/validate-finding.shNotes
cairo-auditorvulnerability patterns + held-out evals.Quick checks run
scripts/audit-extraction/validate-finding.shSummary by CodeRabbit
New Features
Documentation
Chores