Enforce ruff in CI + precommit, bound complexity, and clear lint baseline - #346
Draft
lelia wants to merge 4 commits into
Draft
Enforce ruff in CI + precommit, bound complexity, and clear lint baseline#346lelia wants to merge 4 commits into
lelia wants to merge 4 commits into
Conversation
Ruff already ran in CI, but only as a job inside the Unit Tests workflow,
so it inherited that workflow's path filter and never saw .hooks/,
benchmarks/, or tests/e2e/. Move it to its own unconditional Lint
workflow, which also keeps it usable as a required status check.
Add ruff to pre-commit so violations surface before CI. The hook runs
ruff out of the project environment rather than the upstream mirror, so
the version stays pinned in one place; Dependabot has no pre-commit
ecosystem and would never update a mirror's rev.
Expand the rule set beyond E/F/I to cover bug classes that matter for a
CLI other people run in their pipelines, and fix every resulting
violation so the baseline is clean rather than suppressed.
Behaviour changes worth calling out:
- Package.created_at used str.strip(" (Coordinated Universal Time)"),
which treats its argument as a set of characters, not a suffix. It ate
a leading "T" from "Tue ..." and a trailing "T" from timestamps that
carried no suffix at all. Now uses removesuffix.
- Every requests call in the plugins and the GitLab client now passes an
explicit timeout. requests blocks forever by default, so a hung
notification could wedge the pipeline the CLI reports into.
- Two asserts became real checks. assert is stripped under python -O, so
neither guard survived an optimised interpreter.
- config.py logs through the socketcli logger instead of the root
logger, so its messages honour the configured level and format.
- A stray debug print in the SBOM artifact loop became a log.debug call;
it was writing to stdout, which carries machine-readable output.
- Closures defined inside loops in alert_selection and messages were
hoisted and now take their inputs explicitly.
Complexity is bounded by C901 (max 12) and PLR0913 (max 8). The 20
functions over the limit today carry an explicit noqa; RUF100 fails the
build once a suppression goes stale, so the list can only shrink.
E501 and W291/W293 are left to ruff format rather than duplicated in the
linter: everything the formatter cannot reflow is a string literal, and
the PR-comment markup depends on trailing double-spaces as Markdown
hard line breaks.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`ruff format` normalises string quotes to double, so `__version__` in socketsecurity/__init__.py went from single to double quotes. Five places parsed or rewrote that line assuming single quotes: - version-check.yml stripped only `'`, so it read the version as `"2.7.2"` (quotes included) and failed to parse it. This is what broke on the PR. - build_container.sh and build_container_flexible.sh would have produced a Docker tag containing literal quote characters. - deploy-test-pypi.sh both read the version and rewrote it with a sed that matched single quotes only, so the rewrite would silently no-op. - .hooks/sync_version.py read either quote style but always wrote single quotes, so it and the formatter would have rewritten the same line back and forth on every commit. Readers now strip both quote characters and the hook writes double quotes to match the formatter. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
lelia
marked this pull request as draft
September 7, 2026 18:27
CodeQL flagged `"github.com" in diff_url` as incomplete URL substring sanitization. Looking at what diff_url actually holds makes the finding more interesting than a sanitization gap. diff_url is always a Socket dashboard link, built in Core as `https://socket.dev/dashboard/org/{org_slug}/diff/...` (or the equivalent sbom URL). Its host is always socket.dev and it carries no SCM information -- which is exactly what the comment three lines below the check already said. The only variable part is the org slug, so the sniff could only ever fire when a Socket org slug happened to contain "github", "gitlab" or "bitbucket". Such an org got a link to a repository host it may not use; everyone else fell through to the Socket file view. The branch was also almost unreachable: CliConfig declares `scm` with a default of "api", so `hasattr(config, "scm")` is true for every real config and the elif never runs. It was observable only for a config object carrying `repo` but no `scm`, since the URL builders all require a truthy config -- with `config=None` the sniffed value was computed and then discarded. Replaced with `getattr(config, "scm", None) or "api"`, which handles a missing config, a config without the attribute, and an empty value. Adds tests for get_manifest_file_url, which had none: GitHub, GitHub Enterprise, GitLab, self-hosted GitLab, Bitbucket, the Socket fallback, build-agent prefix stripping, and multi-manifest paths. The three org-slug cases are regression guards, confirmed to fail against the old implementation. Removing the dead branch drops the function under the complexity limit, so RUF100 required its `# noqa: C901` be removed. The backlog is now 19. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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
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.
Ruff already ran in CI, but as a job inside the Unit Tests workflow, so it inherited that workflow's
paths:filter and never saw.hooks/,benchmarks/, ortests/e2e/. This moves it to its own unconditionalLintworkflow, adds a pre-commit hook, bounds function complexity, and clears every resulting violation so the baseline is clean rather than suppressed.ruff checkandruff format --checkare both green across all 94 files. 523 tests pass.Enforcement
Lintworkflow, unconditional on every PR. Not path-filtered: a filtered workflow reports "not run" rather than "passed", which blocks any PR that doesn't touch the filtered paths if it's made a required check.astral-sh/ruff-pre-commitmirror. Dependabot has no pre-commit ecosystem and will never update a mirror'srev:, so a mirror would drift from the pinnedruff==0.16.4and produce the worst hook failure mode — clean locally, red on the PR.make lintnow mirrors CI exactly.Complexity
C901at max 12,PLR0913at max 8.PLR0912andPLR0915were evaluated and rejected on evidence:PLR0912duplicatesC901on 18 of 22 hits, andPLR0915's only unique catch iscreate_argument_parser— 116 statements but perfectly flat.max-args = 8sits at the real gap in this codebase: everything is ≤7 arguments exceptrun_reachability_analysisat 27.The 20 functions over the limit today carry an explicit
# noqarather than a blanketper-file-ignores, so new complex functions in the same files are still caught.RUF100fails the build once a suppression goes stale, so the backlog can only shrink — it already fired once during this work, when a refactor dropped_build_reachability_indexunder the limit. The worst remaining ismain_codeat complexity 109 / 424 statements.Bugs fixed
These are behaviour changes, not style:
Package.created_atwas truncating timestamps.str.strip(" (Coordinated Universal Time)")treats its argument as a set of characters, not a suffix."Tue Jan 15 ..."lost its leadingT, and any timestamp without that suffix lost a trailingT. Now usesremovesuffix.requestsblocks forever by default, so an unresponsive endpoint could hold a run open until the CI job itself timed out. All now pass an explicit 30s timeout. The existing GitLab tests caught this as a signature change and were updated.python -O.assertis stripped in optimised mode. One was a real check on the org slug and now raises; the other was dead weight and was removed.printwas writing to stdout, which also carries SARIF. Nowlog.debug.config.pylogged through the root logger, so its messages ignored the CLI's configured level and format. Now uses thesocketclilogger like the rest of the package.alert_selection.pyandmessages.pycaptured loop variables by reference. Not live bugs — they were called within the same iteration — but they were hoisted and now take their inputs explicitly.Formatting
ruff formatis enforced from this PR onward. The formatting pass is bundled into the same commit as the lint fixes because they touch the same lines and can't be cleanly separated after the fact, so.git-blame-ignore-revsis added with instructions but no SHA — blame-ignoring this commit would also hide the real fixes above.CodeQL:
py/incomplete-url-substring-sanitization— fixedCodeQL flagged
"github.com" in diff_urlatmessages.py:80. The alert pre-exists onmain; it surfaced here because the formatter touched that line.Digging in turned up something more useful than a sanitization gap.
diff_urlis always a Socket dashboard link, built inCoreashttps://socket.dev/dashboard/org/{org_slug}/diff/.... Its host is alwayssocket.devand it carries no SCM information — which is exactly what the comment three lines below the check already said. The only variable part is the org slug, so the sniff could only fire when a Socket org slug contained "github", "gitlab" or "bitbucket". Those orgs got a link to a repository host they may not use; everyone else fell through to the Socket file view.The branch was also nearly unreachable:
CliConfigdeclaresscmwith a default of"api", sohasattr(config, "scm")is true for every real config and theelifnever runs. It was observable only for a config carryingrepobut noscm— withconfig=Nonethe sniffed value was computed and then discarded, since every URL builder also requires a truthy config.Now
scm_type = (getattr(config, "scm", None) or "api").lower(), which covers a missing config, a config without the attribute, and an empty value.get_manifest_file_urlhad no test coverage. Added 18 cases: GitHub, GitHub Enterprise, GitLab, self-hosted GitLab, Bitbucket, the Socket fallback, build-agent prefix stripping and multi-manifest paths. The three org-slug cases are regression guards — I verified they fail against the old implementation rather than assuming they would.Removing the dead branch dropped the function under the complexity limit, so
RUF100required its# noqa: C901be deleted. Complexity backlog: 20 → 19.TODO: four judgment calls still to confirm
The baseline is clean partly because four rule families are deliberately not enforced. Each was decided against actual call sites, but these are policy choices that should be signed off rather than inherited:
TRY400(35 sites) — would rewriteexcept APIFailure as e: log.error(...)tolog.exception(), dumping tracebacks into customer CI logs for expected conditions like a missing config file. Rejected as making error reporting worse. Confirm, or accept the noisier logs.E501+W291/W293— left toruff format, which owns line length (120) and whitespace. Everything the formatter can't reflow is a string literal, and the PR-comment markup relies on trailing double-spaces as Markdown hard line breaks. Confirm, or take on ~43 hand-wrapped strings.N(naming) —N815wanted to rename the camelCase fields that mirror Socket API JSON keys (supplyChain,topLevelAncestors,manifestFiles), andN818wanted to renameAPIFailure/APIResourceNotFound, which this repo's own CI smoke test imports. Both would be breaking. Confirm.SIM108/PERF401/S603/S607— listed underignorewith reasons inpyproject.toml.SIM108would delete an explanatory comment;PERF401would push multi-line dict literals into generator expressions;S603/S607fire on everysubprocesscall and this CLI shells out togitand the coana binary by design. Confirm.Public Changelog
N/A
Ref: CE-451
Note
Medium Risk
Touches CI required checks, broad lint/format churn, and several CLI/runtime paths (HTTP notifications, config exit/logging, package timestamps) that affect customer pipelines.
Overview
Release 2.7.2 ships alongside a full Ruff enforcement story: linting moves out of the path-filtered Unit Tests workflow into a dedicated, unconditional
Lintworkflow (ruff check+ruff format --check), with matching pre-commit hooks andmake lint/make hookstargets.pyproject.tomlexpands the rule set (bugbear, bandit, complexityC901/PLR0913,RUF100, etc.), documents intentional ignores, and clears the baseline across the repo.Runtime fixes (not just style):
Package.created_atnow usesremovesuffixinstead ofstripso timestamps are not mangled; outbound Slack/Teams/Jira/webhook/GitLab calls get a 30srequeststimeout so CI cannot hang forever;config.pylogs via thesocketclilogger andsys.exit; duplicate SBOM packages log at debug instead of stdout; and guards that relied onassertare replaced or removed sopython -Ostill behaves correctly.Docs (
CONTRIBUTING.md,CHANGELOG.md) and.git-blame-ignore-revs(placeholder for future format-only SHAs) support the new workflow.Reviewed by Cursor Bugbot for commit ea36905. Configure here.