Repository navigation
Conversation
codeql.yml builds PostgreSQL, SVS, and pgvector from source under docs/build_guide/ before tracing this extension's own build, so the C/C++ extractor records every header transitively included during that trace -- including these vendored, untracked dependency trees. Of the 56 open alerts triaged, 7 (cpp/commented-out-code, cpp/fixme-comment, cpp/irregular-enum-init) sit entirely inside PostgreSQL core's own headers under pgsql_install/include/server/, none of which is this project's source. Add paths-ignore for all of docs/build_guide/'s build-output subdirectories, not just the one that happened to trigger an alert this run, so SVS's or pgvector's own vendored headers don't produce the same class of noise later. Signed-off-by: Olasoji <olasoji.denloye@intel.com>
GitHub Actions requires step 'with:' values to be scalars (they become environment variables), so the YAML sequence form failed workflow validation outright -- 0 jobs ran, and GitHub flagged it as a broken workflow file on push even though this workflow's own push trigger doesn't match non-main branches, since GitHub validates every workflow file present in a pushed commit regardless of whether its triggers would fire. codeql-action parses this input by splitting on newlines, so a '|' block scalar with one path per line is the correct form for a list-like input passed directly on the init step (as opposed to a separate codeql-config.yml file, which is parsed by the action's own code and does support real YAML lists for the same key). Signed-off-by: Olasoji <olasoji.denloye@intel.com>
paths-ignore is not a recognized input to codeql-action/init at all, in any form -- confirmed from the actual run log's own warning: "Unexpected input(s) 'paths-ignore', valid inputs are [... 'config-file', 'config', ...]". The previous commit's fix (converting it to a multi-line string) only silenced the workflow-schema error that came from using a YAML sequence directly under 'with:'; the action itself still ignored the setting entirely and the dependency paths were never actually excluded. The real mechanism is 'config' (or a separate codeql-config.yml via 'config-file'): a nested YAML document that codeql-action parses with its own schema, which does support real lists for paths-ignore. Move the exclusion there instead. Signed-off-by: Olasoji <olasoji.denloye@intel.com>
matt-welch
left a comment
matt-welch
left a comment
There was a problem hiding this comment.
Thanks for cleaning this up. The directory list is accurate (matches exactly what install_postgres.sh, build_svs.sh, and build_pgvector_vanilla.sh create per docs/build_guide/config), and the config: inline YAML is valid syntax for codeql-action/init.
I got curious about how paths-ignore interacts with our build-mode: manual traced build, since that combination isn't the usual case the CodeQL docs walk through. So I built the dependency stack locally and ran CodeQL CLI 2.27.1 (matching our CI version) through the same init/trace/finalize steps, once with this PR's paths-ignore config and once without, just to see what actually ends up in the database either way.
Both runs came back identical: 485 files, same total size, no difference in the file lists. The headers under docs/build_guide/pgsql_install and docs/build_guide/svs_install (the two vendored dirs the traced build actually touches) show up in both. It looks like paths-ignore doesn't affect extraction when the build is traced like ours is, so this version of the config probably won't change what gets scanned.
Also noticed the other three directories in the list (postgres, ScalableVectorSearch, pgvector-vanilla) didn't show up in either run. The traced build only ever points at the installed output dirs, not the raw source checkouts, so those three wouldn't have mattered either way.
This fix may need to happen on the build/trace side instead of through this config option. Happy to share the local repro if it'd help.
|
Quick follow-up on my comment above. I wanted to check this more directly than just comparing extracted file lists, so I ran the actual Both runs produced the same 56 alerts, matching exactly in rule, file, and line. 7 of those 56 are real findings sitting in vendored PostgreSQL headers under Then I checked that against the real analysis GitHub already has on file for That's about as direct as this can get: not extracted-file counts, but the actual alerts a reviewer would see, reproduced twice and confirmed against the live repo's own existing scan. There is a real basis for wanting this fix (those 7 alerts in vendored code are genuinely there), but this particular change doesn't clear them. Happy to share the SARIF output if it'd help track down a fix on the build/trace side. |
2 participants
Description
codeql.yml builds PostgreSQL, SVS, and pgvector from source under docs/build_guide/ before tracing this extension's own build, so the C/C++ extractor records every header transitively included during that trace -- including these vendored, untracked dependency trees.
This PR add paths-ignore for all of docs/build_guide/'s build-output subdirectories, so SVS's or pgvector's own vendored headers don't produce the same class of noise later.
Related Issues
Type of Change
Pre-Merge Checklist
Build
makecompletes without errors or warningsmake installcompletes successfullyTests
make installcheck) and TAP tests (test/t/) pass with no failurestest/sql/and/ortest/t/test/modules/: it builds and passes (make -C test/modules/<module> installcheck)Documentation
docs/updated if architecture or usage changedTesting Notes