static-analysis: convert the semgrep scan fan-out to a dynamic workflow - #231
Conversation
Review — static-analysis: semgrep scan → dynamic workflowStatic review only: every execution attempt in this environment was denied ( Clean dimensions. Verifiers: FindingsP2 — the PR description does not describe this diff. (a) " P3 — P3 — P3 — P4 — P4 — P4 — P4 — P4 — P4 — stale post-fan-out text in P4 — P4 — P4 — P4, low confidence, unverified — P4 — Notes, not findings
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
be2a26b to
c98999f
Compare
|
Addressed all three review comments in Argument validation. Clone URL. Quoted as Dedup key. Six new assertions, 54 total. Each guard was mutation-tested: removing the array check, the |
c98999f to
f82757e
Compare
|
Addressed all six findings from the second review in P3 verdict-lookup collision. Correct diagnosis, and it was the same bug my per-language dedup comment describes, one level up. Each scan now carries an P3 ruleset string validation. P3 P4s. The reconciliation now separates a surplus explained by a Five new assertions, 59 total, and both P3 fixes are mutation-tested. |
f82757e to
d6fe73d
Compare
|
Fixed the P2 and both P3s in P2 — Both now go through a shared Five new assertions: command substitution in P3 — P3 — metadata severity table. Column header is now 64 assertions. The P2 guard is mutation-tested in both directions: removing it fails four assertions, and replacing it with an over-strict allowlist fails the acceptance one. |
d6fe73d to
03fbeca
Compare
|
Fixed the P2 in P2 — cross-language-only plan was impossible. Correct, and it was a design error rather than a slip: I reasoned that baseline and third-party rulesets could not be "scoped" to a scan without language keys, when in fact they are the rulesets that never get scoped — they run unscoped over the whole target and form a complete The throw now fires only when the plan holds no entries at all. Four assertions replace the two that enshrined the old behaviour: a cross-language-only plan spawns exactly one unit, generates both commands unscoped, and reports both in Not taken this round, deliberately, and I agree they are real:
68 assertions. |
03fbeca to
d0e1512
Compare
|
Round 5 fixes in the amended head. P2 — important-only Nor can that filter be ported: semgrep's SARIF carries only P3 — literal P3 — the Not taken: the 70 assertions. |
d0e1512 to
da9221f
Compare
|
Round 6 fixes in P2 — false mismatch on the normal case. Right, and I verified it: P2 — unconditional P3 — P3 — Not taken: the six P4s, and the 70 assertions, unchanged — this round was all doc-side except the agent file. |
3d85cf6 to
718821e
Compare
…rged SARIF, clear stale raw output Three defects flagged by kz-tob on #231. The workflow hardcoded SKILL_DIR as a repo-relative path, so every scripted command only resolved inside a checkout of this repo. A marketplace install runs with the user's own project as cwd and the scan phase would have found no run-scans.sh at all. Resolved at runtime instead, folded into the Detect phase, following the cascade variants.js already uses. Each candidate ends at scripts/run-scans.sh, which makes a stale install self-excluding: verified against the 1.2.2 install on disk, which ships merge_sarif.py and nothing else, and the glob correctly declines to bind to it. An unresolved directory throws rather than leaving an agent to compose semgrep commands by hand. In important-only mode both the workflow and scan-workflow.md told the agent to apply the scan-modes.md jq filter to the merged SARIF. That filter reads .results[].extra.metadata, which SARIF does not have, so it exits with "Cannot iterate over null" and results.sarif stayed unfiltered while the JSON side was filtered. The metadata is not recoverable from SARIF, but finding identity is: (check_id, path, start.line) and (ruleId, uri, region.startLine) match field-for-field, confirmed against real semgrep output, and it is the same triple the merge already dedups on. merge_sarif.py --important keeps the findings the JSON filter kept and fails rather than filtering if any scan has no *-important.json beside it, since a partial key set would drop real findings from the deliverable. run-scans.sh never cleared raw/. merge_sarif.py globs every *.sarif there, so a rerun into a reused output directory that dropped a ruleset still merged the previous run's output for it. Tests: 58 shell assertions (+3), 46 workflow assertions (+13) with three new mutations, and 16 pytest cases for merge_sarif.py. Each fix is mutation-tested; reverting any one of them turns the suite red.
…runner Two entry points over one implementation. SKILL.md keeps its gated five-step path, where the user reviews and edits the ruleset list before anything runs. workflows/semgrep-scan.js runs the same scan end to end without stopping. Both read the same references/, so a ruleset added to rulesets.md reaches both at once. This is the shape variant-analysis uses. The workflow sits at the plugin root beside the four already on main and ships as /static-analysis:semgrep-scan. Four phases: Detect resolves the output directory and profiles languages and Pro, Select reads references/rulesets.md and writes rulesets.json, Scan runs the script, Report post-filters, merges and summarizes. Step 4 is skills/semgrep/scripts/run-scans.sh, not a fan-out of subagents. Nothing in the scan phase needs judgement: the agents would run fixed commands and report $? and a jq count. Exit codes now stay with the processes that produced them and finding counts come from the JSON they wrote, so no phase reports on work a later phase has to go behind and re-verify. --metrics=off, the --include scoping, the output-directory --exclude and the severity flags are properties of the script. Cross-language rulesets run once rather than once per language and never take --include; a ruleset already in baseline is dropped from its language; language keys fold onto a canonical name, so js and javascript are one unit; two spellings of one repository collapse to one clone. Parallelism is the script's --jobs. The workflow does not stop for ruleset approval. None of main's four ask the user anything, and invoking one with a target is the opt-in. The scan is read-only over the target -- no --autofix or --fix is ever passed, every write lands inside the output directory, and the script refuses to run when the output directory is the target. Semgrep rules are declarative YAML, so pointing --config at a cloned rule repository executes nothing from it. The gate was scope confirmation rather than protection from a dangerous action, and what ran is recorded in rulesets.json and scans.json either way. A deliberate change to a security skill's stated policy, not an oversight; the gated path remains for when the ruleset selection is the thing that matters. Fixes four defects the old prose path carried: allowed-tools omitted the tool Step 4 needed; --severity MEDIUM/HIGH/CRITICAL is rejected by semgrep, so important-only mode never ran; each scanner deleted the shared repos/ clone while others were still reading it; and the scanner agent declared its tools as a comma string where the loader expects a list. Two bugs the new suites found while being written. `eval "$cmd" &` reports exit status 1 whatever the command exited with, which marked every failed scan as a success; commands are built as an argv array and executed directly, which also leaves no quoting surface. And the target was resolved with `cd && pwd` while the output directory was not, so on any path crossing a symlink -- every path under /var on macOS -- both the equality check and the inside-the-target test missed, and the run scanned its own cloned rule repositories. tests/run_scan_tests.sh covers the script: command generation via --dry-run, and execution, exit codes and clone failures against stub semgrep and git binaries. tests/workflow-harness.js compiles the workflow with stubbed globals and asserts that a relative target, an output directory equal to the target, a dead phase and a failed scan each stop the run rather than reach the report as an empty result; --self-test mutates the workflow six ways and requires every mutation to turn a scenario red. Both are hermetic, reach no network, and CI's existing shell-suite discovery runs them, so no workflow file changes. Removes references/scanner-task-prompt.md and the no-Workflow fallback it served. There is no hand-rolled path and no second implementation in prose.
…rged SARIF, clear stale raw output Three defects flagged by kz-tob on #231. The workflow hardcoded SKILL_DIR as a repo-relative path, so every scripted command only resolved inside a checkout of this repo. A marketplace install runs with the user's own project as cwd and the scan phase would have found no run-scans.sh at all. Resolved at runtime instead, folded into the Detect phase, following the cascade variants.js already uses. Each candidate ends at scripts/run-scans.sh, which makes a stale install self-excluding: verified against the 1.2.2 install on disk, which ships merge_sarif.py and nothing else, and the glob correctly declines to bind to it. An unresolved directory throws rather than leaving an agent to compose semgrep commands by hand. In important-only mode both the workflow and scan-workflow.md told the agent to apply the scan-modes.md jq filter to the merged SARIF. That filter reads .results[].extra.metadata, which SARIF does not have, so it exits with "Cannot iterate over null" and results.sarif stayed unfiltered while the JSON side was filtered. The metadata is not recoverable from SARIF, but finding identity is: (check_id, path, start.line) and (ruleId, uri, region.startLine) match field-for-field, confirmed against real semgrep output, and it is the same triple the merge already dedups on. merge_sarif.py --important keeps the findings the JSON filter kept and fails rather than filtering if any scan has no *-important.json beside it, since a partial key set would drop real findings from the deliverable. run-scans.sh never cleared raw/. merge_sarif.py globs every *.sarif there, so a rerun into a reused output directory that dropped a ruleset still merged the previous run's output for it. Tests: 58 shell assertions (+3), 46 workflow assertions (+13) with three new mutations, and 16 pytest cases for merge_sarif.py. Each fix is mutation-tested; reverting any one of them turns the suite red.
0375a20 to
2e8f747
Compare
|
Fixed all three in Merged SARIF unfiltered in important-only mode. Confirmed against real output rather than by reading: the old instruction run against a merged SARIF gives exactly The filter cannot be ported: semgrep's SARIF carries no It derives the required Hardcoded Stale 58 shell assertions (+3), 46 workflow assertions (+13) with three new mutations, and 16 pytest cases for One correction: the review says the round-5 identity-key mechanism "reverted." I find no trace of it in this branch's history, so I treated it as never having landed rather than as a regression. Does not change the fix. |
…rged SARIF, clear stale raw output Three defects flagged by kz-tob on #231. The workflow hardcoded SKILL_DIR as a repo-relative path, so every scripted command only resolved inside a checkout of this repo. A marketplace install runs with the user's own project as cwd and the scan phase would have found no run-scans.sh at all. Resolved at runtime instead, folded into the Detect phase, following the cascade variants.js already uses. Each candidate ends at scripts/run-scans.sh, which makes a stale install self-excluding: verified against the 1.2.2 install on disk, which ships merge_sarif.py and nothing else, and the glob correctly declines to bind to it. An unresolved directory throws rather than leaving an agent to compose semgrep commands by hand. In important-only mode both the workflow and scan-workflow.md told the agent to apply the scan-modes.md jq filter to the merged SARIF. That filter reads .results[].extra.metadata, which SARIF does not have, so it exits with "Cannot iterate over null" and results.sarif stayed unfiltered while the JSON side was filtered. The metadata is not recoverable from SARIF, but finding identity is: (check_id, path, start.line) and (ruleId, uri, region.startLine) match field-for-field, confirmed against real semgrep output, and it is the same triple the merge already dedups on. merge_sarif.py --important keeps the findings the JSON filter kept and fails rather than filtering if any scan has no *-important.json beside it, since a partial key set would drop real findings from the deliverable. The merge command blocks in SKILL.md and scan-workflow.md show both modes, so copying the block without reading the paragraph under it cannot produce an unfiltered deliverable. run-scans.sh never cleared raw/. merge_sarif.py globs every *.sarif there, so a rerun into a reused output directory that dropped a ruleset still merged the previous run's output for it. Tests: 58 shell assertions (+3), 46 workflow assertions (+13) with three new mutations, and 16 pytest cases for merge_sarif.py. Each fix is mutation-tested; reverting any one of them turns the suite red.
2e8f747 to
1969efd
Compare
kz-tob
left a comment
kz-tob
left a comment
There was a problem hiding this comment.
changes requested were completed
#231 bumped the plugin to 1.3.0 on main after this branch had already done the same, so merging main left HEAD and the merge base equal and the version-increment check failed.
) * fix(codeql): make the skill's guards real, add tests, trim the prose Every verification step piped its command to a formatter and used the pipeline's exit status, which without pipefail belongs to the formatter, not the command. Nine sites. The sharpest was the arm64e detection: `EXIT_CODE=$?` after `| tee` compared against 137, a value it could never hold, so the check underpinning Essential Principle #5 and three Rationalizations could not fire. The build workflow now runs check_db_quality.py as its Step 4 exit condition rather than describing metrics it never compared to a threshold, and suite generation aborts on zero resolved queries. Two blocks that were invalid bash — an else branch containing only comments — are fixed. Log helpers move to scripts/build_log.sh, sourced by the workflow and by the three reference docs that use run_logged. They were previously defined in one markdown file and used from three others, so those blocks failed standalone. As the skill's first .sh file it is also the first thing here that `make shell` lints. Sourcing it now checks the log is writable: under pipefail an unwritable log made tee's failure the pipeline's, so run_logged returned 1 for a build that succeeded and the method ladder walked to --build-mode=none blaming CodeQL. Suite generation moves to scripts/generate_suite.sh, which takes the mode as its argument. Both modes had been copy-pasted into two reference docs sharing some 25 lines of identical scaffolding — guards, the third-party pack loop, the excludes, the verification call — none of it lintable where it sat. The tests now run the script instead of extracting bash from markdown, and one of them fails if either doc inlines a generation block again. check_db_quality.py counts project files under the source root recorded in codeql-database.yml instead of against a hardcoded prefix list that only knew where macOS keeps its toolchain. src.zip stores each file at its absolute path minus the leading separator, so the recorded root is a prefix of the project's entries and of nothing else — verified against a database built by CodeQL 2.25.6. It also resolves the summary-versus- severity preference per extractor: a single `extractor-failures: 0` used to suppress the fallback for every other language in the database. run-analysis.md had its own database discovery loop, which SKILL.md states the workflows do not restate. It also lacked SKILL.md's `codeql resolve database` filter, so a marker file left by a failed build could be selected as a database. It now uses the canonical block. Twenty-odd snippets across language-details, threat-models, and performance-tuning created and analysed a literal `codeql.db` in the working directory — the exact shortcut SKILL.md lists as a Rationalization to Reject, and one its Success Criteria forbid. They use "$DB_NAME" under $OUTPUT_DIR. Adds six hermetic test suites needing no CodeQL install, plus the first tests that execute build_log.sh rather than reading it. They cover the shell in every markdown block, both suite generators, the quality thresholds, the .qls templates, and the exit-status claim the build ladder rests on. Corrects the run-all coverage figures against cpp-queries 1.8.0: the pack holds 515 alert queries, not 510, and run-all leaves 307 of them unrun, not 302 — 13 of those are Security/CWE queries, which the prose described as harmless refactoring metrics. Trims the skill's markdown from 2867 lines to 2748, and SKILL.md from 269 to 257. Out: the When NOT to Use section that #216 dropped repo-wide, five Essential Principles that restated their own Rationalization, two literal prompt mock-ups, three Reference Index rows duplicating the workflow table above them, three identical qlpack.yml blocks, and code fences in performance-tuning and threat-models that each carried a single flag. Replaces three approval prompts with one confirmation gate, and drops allowed-tools entries for tools that no longer exist. Bumps static-analysis to 1.3.0. * fix(codeql): address review findings on shell-block scope and suite claims Each markdown block runs in a fresh shell, so scalars and sourced functions cross it no better than arrays do. build-database.md now says to re-source build_log.sh and re-set DB_NAME in every block using run_logged: without it run_logged exits 127, the method ladder reads that as a failed build method, and it walks to --build-mode=none having never invoked CodeQL. run-analysis.md Step 4 re-establishes DB_NAME, RAW_DIR and SUITE_FILE for the same reason -- under set -u it aborted with "unbound variable" before analysis started. create-data-extensions.md was the third caller of database discovery and still used a bare find for codeql-database.yml, which selects a marker left by a build killed mid-run; the queries then return nothing and Step 3 reports coverage as adequate. It now uses find_databases.sh like the other two. quality-assessment.md sources build_log.sh in the Collect Metrics block, so a failed quality gate is recorded rather than silently dropped by a command-not-found, and the raised-threshold override logs inside the if -- it previously wrote "raised to 15%" even when the re-run still failed. generate_suite.sh no longer describes the run-all suite as every security, experimental and quality query: it imports two suites totalling 219 of the pack's 515 alert queries, as run-all-suite.md documents. Changed in the doc template too, which test_generation_scripts.py pins to the script's output. The two remaining review findings were already fixed on this branch and needed no change: quality-assessment.md assigns ERROR_RATIO from the script's JSON before reading it, and extractor_error_count() resolves the summary-versus- severity preference per extractor rather than once for the tree. * style(codeql): trim comments in the shared shell scripts build_log.sh carried more comment than code. The consequence of an unwritable log -- the ladder walking to --build-mode=none after a build that succeeded -- was stated twice within ten lines; it is stated once now. find_databases.sh and generate_suite.sh get the same treatment. Comments only: the diff contains no code lines. 435 tests pass unchanged. * feat(codeql): ship /static-analysis:codeql-build as a dynamic workflow The build is the part of this skill with real judgement and no user in it: try a method, read the failure, apply a fix, retry, escalate. That loop now runs unattended as workflows/codeql-build.js, beside the four workflows already on main. Three phases. Detect resolves the output directory and profiles the language, build system and macOS arm64e state. Build walks the ladder. Assess runs check_db_quality.py and applies the improvements from quality-assessment.md before re-running it. The ladder is deterministic and lives in the script; diagnose-fix-retry for a single rung lives in that rung's agent, where the build output it has to read already is. The agent is told not to escalate itself -- the caller owns the order, so a rung that fails is a result rather than a licence to try something else. Go and Swift never reach Method 4, which they reject outright; an arm64e Mac starts at 2m rather than spending two rungs to reach the same SIGKILL; an interpreted language does one extraction and no ladder at all. Nothing is asked. Every method failing returns no-method-succeeded, and a database that built but sits below the quality threshold returns built-below-threshold with its metrics. Whether the remaining extractor errors are confined to code nobody needs analysed is the caller's call, so the assess phase is told not to raise --max-error-ratio to make its own gate pass. A build command exiting 0 is not a database: every rung is confirmed with codeql resolve database before it counts, because finalize after a failed trace-command leaves one that resolves and holds nothing. tests/codeql_build_harness.js compiles the workflow with stubbed agents and asserts the ladder and the guards; --self-test mutates it six ways and requires every mutation to turn a scenario red. run_codeql_build_tests.sh wraps both so CI's existing shell-suite discovery runs them, with no workflow file changes. SKILL.md documents it as the unattended alternative and keeps the manual path. Database selection, analysis planning and data extensions stay in the session. * refactor(codeql): stop the docs recomputing what check_db_quality.py reports Collect Metrics parsed baseline-info.json with an inline python3 -c into BASELINE_LOC, then read .baseline_loc out of the checker's JSON into DB_LOC in the same block -- the same number, computed two ways, logged twice under two labels. The block's own comment said "Everything downstream reads $QUALITY_JSON rather than recomputing" while the recount sat sixteen lines above it. The inline parse and the print-baseline call before it are gone; the checker is the only thing that counts now, and the Quality Criteria table cites it rather than a command the doc no longer runs. Log Assessment read five variables out of Collect Metrics' shell. It runs in its own, so they expanded to empty and the log recorded "Baseline LoC:" with no number -- the same shape as the dangling ERROR_RATIO the first review found. It re-sources the helpers and re-reads the metrics, and no longer prints an expected-file count it cannot see. test_shell_blocks.py's embedded-python guard required three matches and there are two now, which is the guard working: removing the last sample of a construct must fail rather than silently leave the extractor untested. The floor moves to two, with the reason recorded. * test(codeql): drop test_suite_resolution.py, which never ran in CI Its six tests needed the CodeQL CLI and codeql/cpp-queries; CI installs neither, so every one of them reported as a skip on every run. Confirmed by reproducing CI's environment locally -- with codeql off PATH the directory gives 428 passed, 135 skipped, matching the job log exactly. Removing it leaves 428 passed, 129 skipped, and the remaining skips are per-block parametrisations of test_shell_blocks.py whose test functions do run for other blocks. What goes with it: the only check that resolved the suite templates against a real CodeQL rather than a fake one. run-all-suite.md's coverage claims -- that run-all is not the whole pack, that important-only reaches queries run-all does not -- are now prose nobody verifies, so the doc carries the command to re-derive them instead of pointing at a test file that is gone. test_generation_scripts.py's docstring no longer claims a sibling covers real-CLI resolution. * fix(codeql): Rust supports --build-mode=none; say so The language table left Rust as "check your CLI — not listed either way" and the Overview's three categories did not cover it at all, so a reader had no route for a Rust project and codeql-build.js resolved the ambiguity silently by putting Method 4 on its ladder. Settled against CodeQL 2.25.6 rather than the help text: `codeql database create --language=rust --build-mode=none` exits 0 and writes a database with `finalised: true`. Go, for contrast, fails immediately with "Go does not support the none build mode". So `--help` omits Rust the same way it omits C/C++, which the Overview already warned about; Rust joins that category and the ladder in codeql-build.js was right. * refactor(codeql): one prose copy of the quality-gate exit codes, not two check_db_quality.py's exit contract was written out four times: the script's own docstring, codeql-build.js's assess prompt, build-database.md Steps 4-5, and quality-assessment.md's Enforce the Thresholds. The first two earn it -- one is the source of truth, the other is an agent prompt that cannot read a docstring. The two prose copies are one too many. build-database.md keeps the part a reader needs at that moment (exit 1 is not a judgement call, exit 3 is) and defers the table to quality-assessment.md, which it already links and which is where someone looks for gate detail. The raised-threshold rationale there loses two lines it did not need. * refactor(codeql): assess phase reads the exit-code table instead of copying it The Assess prompt spelled out all four exit codes while its sibling phases point at a file -- Select says to read rulesets.md rather than choose from memory, and each build rung is sent to build-fixes.md. It now reads "Enforce the Thresholds" in quality-assessment.md the same way, keeping inline only the two facts that decide what it does: exit 1 is not overridable, exit 3 is a heuristic. That leaves one prose description of the contract instead of two, and a change to the script's exits reaches the agent without a second edit. Also aligns the ladder comment with the Rust finding: --help omits C/C++ and Rust, not just C/C++. * fix(codeql): pass --format=json, the flag check_db_quality.py defines The workflow's Assess phase ran `check_db_quality.py --json`. The script takes --format {text,json}, so argparse exited 2 before reading the database, and 2 is the one exit code ASSESS_SCHEMA does not describe. test_script_flags.py checks the class: it reads each script's accepted flags from its own --help and verifies every invocation across the plugin's .md and .js. The bug shipped because test_shell_blocks.py scans the skill tree and codeql-build.js sits outside it. * test(codeql): scan every block in one test instead of parametrizing over all of them Seven tests parametrized over all 65 bash blocks, which is 455 cases for seven assertions, and three of them skipped the blocks that did not qualify. That was 129 skips, and it hid a check that matched no block at all: the unpreserved-pipeline assertion had never run against the skill. Each now scans in one pass and lists every offending file:line, so a run reports all offenders rather than the first. 557 cases down to 118, none skipped. The array collector's empty case is an assertion rather than a skip. * refactor(codeql): point the workflow at build-database.md instead of restating it codeql-build.js carried its own copy of the build procedure: the arm64e detection block verbatim, the build-system command table, every method's invocation, and the output-directory logic. Both copies were live, which is how the workflow came to pass check_db_quality.py a --json flag while the doc had --format=json. Each rung now names its section of build-database.md, the way Method 2m already pointed at macos-arm64e-workaround.md. quality-assessment.md called the checker three times and re-derived its numbers with jq, unzip and grep. check_db_quality.py now reports archive_files and finalised from the two files it already reads, so one call covers the whole assessment. The fresh-shell rule was stated in five places; SKILL.md holds it once and the workflows link to it with the consequence specific to their site. test_section_pointers.py checks what this trade depends on: every "Section" in file.md pointer and every #anchor link must land on a real heading. The repo validator resolves paths, not section names, so a renamed heading would leave the pointers aimed at nothing with every check still green. Prose 1128 -> 1084 lines, codeql-build.js 390 -> 340. * fix(codeql): address PR review findings on skill paths and database selection codeql-build.js hardcoded plugins/static-analysis/skills/codeql, which only resolves in a checkout of this repo. Installed, the first build block sourced a build_log.sh that was not there and exited 127, so every rung of the ladder reported a build failure for a project that would have built. The Detect phase now resolves the directory at runtime from $CLAUDE_PLUGIN_ROOT, $CODEX_PLUGIN_ROOT, or a find over ~/.claude and ~/.codex, accepting a candidate only when scripts/build_log.sh exists. skillDir is a required schema field validated as absolute, so an unresolved path stops the run before the ladder starts. SKILL.md built FOUND_DBS in one bash fence and looped over it in the next. Each fence is a separate shell, so the metadata loop iterated zero times and the selection prompt had no language or creation time to show. The two are now one block. run-analysis.md Step 1 branched only on the zero-database case and fell through to FOUND_DBS[0] for any other count, analysing whichever database find returned first without telling the user there was a choice. It now uses the elif/else shape from create-data-extensions.md and exits with an error when DB_NAME is still unset. Two new checks cover the class rather than the instance. test_shell_blocks.py fails when a block reads an array it does not build. test_section_pointers.py fails on a repo-relative plugins/ path in the workflow, and resolves ${SKILL_DIR} pointers against the skill root so the workflow's file references stay checked: nine of them now, up from two. The rest of the review: run-all-suite.md no longer claims total coverage, which its own measurement section refutes; the analyze block expands optional flags as ${ARR[@]+"${ARR[@]}"}, since bash 3.2 treats an empty array under set -u as unbound; find_databases.sh exits 2 when codeql is absent instead of printing nothing, which auto-detection reads as "no databases, rebuild"; make -j$(nproc) falls back to sysctl -n hw.ncpu on macOS; every . build_log.sh site is || exit 1, as none of those blocks set -e; the Reference Index lists find_databases.sh and generate_suite.sh. * fix(codeql): source build_log.sh in every block that uses its helpers Each fenced block is its own Bash call, so a helper defined in an earlier one is undefined and exits 127. The build ladder reads that as a failed method and walks to the next one, reporting failure for a build that was never attempted. test_shell_blocks.py now fails any block that uses run_logged, log_step, log_cmd, log_result or LOG_FILE without sourcing build_log.sh, with fixtures pinning the detector in both directions. * fix(codeql): gate each step of the Method 3 multi-step build build_log.sh does not set -e, so the four run_logged calls ran regardless of each other: a failed trace-command still reached finalize, and the resulting database resolves while holding nothing, which the ladder reads as success. The steps now chain through if/elif, as 2m-a already does. test_shell_blocks.py fails an ungated `codeql database finalize`. * fix(codeql): stop database discovery reporting none when it found some find_databases.sh resolves each root to an absolute path, so the old -not -path '*/.*' exclusion also matched dotted ancestors: a checkout under ~/.cache or ~/.local had every database filtered out, the script printed nothing and exited 0, and the caller rebuilt from scratch. Prune dotted directories by name instead, with -mindepth 1 so a root that is itself dotted still searches. All three callers read the script through a process substitution, whose exit status is unobservable. Exit 2 (no codeql on this shell's PATH, a fresh shell per block) arrived as an empty list and was reported as "No CodeQL database found" for a project with several. Read it with command substitution and check the status. Tests cover the dotted-ancestor case, the dot-directory-below-root case the fix must not widen into, and a block scanner rejecting a process substitution around the script. * chore(static-analysis): bump version to 1.3.1 #231 bumped the plugin to 1.3.0 on main after this branch had already done the same, so merging main left HEAD and the merge base equal and the version-increment check failed. --------- Co-authored-by: kz-tob <kara.zaffarano@trailofbits.com>
3 participants
Step 4 spawned scanners by prose instruction.
workflows/semgrep-scan.jsnow generatesevery semgrep command from the approved ruleset object, so
--metrics=off, the--includescoping rule, and parallel dispatch are properties of the code. The approved list crosses
into the workflow as
args, so a scan cannot reach a ruleset the user declined. Theapproval gate stays in the main session, which is where it has to be: workflow agents run
in the background and cannot ask anything.
Cross-language rulesets now run once instead of once per language. They scan the whole
target unscoped, so the identical command ran N times and the SARIF merge deduped the copies.
Defects fixed
allowed-toolsnamed the spawn tool wrongly for the paths this skill actually takes. Itnow lists
Agent,Task, andWorkflow:Workflowfor the main path, and both spawnspellings because the name differs by build and the fallback path is the one place where
guessing wrong leaves no path at all.
--severity MEDIUM --severity HIGH --severity CRITICALis rejected by semgrep, whichtakes
INFO,WARNING, orERRORand exits 2 before scanning, so important-only modehas never run. That scale belongs to rule metadata, which the post-filter reads.
repos/and deleted it "after all scans complete". Agents runconcurrently, so they collided on the clone and the first to finish deleted rules the
others were still reading. Cleanup moved to Step 5.
agents/semgrep-scanner.mddeclaredtools:as a comma string where AGENTS.md specifiesa YAML list;
validate_agent_frontmatterchecks only which key is used.semgrep-triageragent that has never existed.Tests
tests/semgrep-scan.test.mjsstubs every agent and runs 121 assertions over the generatedcommands and the result assembly. The negative ones carry the weight: a failed clone, a
ruleset reported
ok: false, and every ruleset held by an agent that died must all fail toreach
scans. Step 5 additionally checks eachscansentry's own json and sarif paths ondisk, because
scansis the agent's own claim and the script has no filesystem access tocheck it.
Adds a
js-testsmake target and CI job, ported fromclaude/git-cleanup-dynamic-workflow-57b146. Both fail when discovery finds nothing andwhen a suite runs without asserting. If that branch merges first, both hunks conflict
trivially.
Verification
make js-tests,make validate,prek run -a, and both loadability checks pass. Thegenerated commands were executed against semgrep 1.168.0 on a Python + Dockerfile fixture
and merged.
make batswas not run locally (not installed; those suites are undermodern-pythonandgh-cli, untouched here).🤖 Generated with Claude Code