feat(safegres): source-level convention linter (C1–C4) in the constructive preset - #1613
Merged
Conversation
Contributor
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
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.
Summary
Adds a source-level convention linter to safegres: house-style rules that read function definitions (
pg_get_functiondef) rather than the catalog, and enables the first four in thesafegres:constructivepreset. This is the ESLint-for-SQL layer we discussed — stable rule codes, per-rule severity, and ESLint/Prettier-style inline waivers with a mandatory reason for the one case we actually allow (dynamic SQL).The lint module lives under
packages/safegres/src/lint/*and is deliberately puresource → findingswith zeropgimports, so it stays mechanically liftable into a standalone@pgsql/lintthe day a second consumer appears. Dependency direction stayssafegres → lint → pgsql-parser; no backward edge.The four rules
no-set-search-pathSET search_pathandset_config('search_path', …)no-variable-conflict#variable_conflictdirectiverequire-qualified-refsno-dynamic-sqlEXECUTE/EXECUTE … USING/FOR … IN EXECUTEC4 can't be proven read-only statically (the executed string is opaque), so every site is flagged and must be waived inline with a categorized reason (
lookup-only/codegen); a reasonless waiver does not suppress it. Waived findings are not dropped — they surface asacknowledged(accepted-risk) findings carrying their reason, off the score.Inline suppressions (ESLint/Prettier style)
A directive with no rule id applies to every convention rule.
Wiring
registry.ts: adds C1–C4 with a newscope: 'function-src'.allAstRulesDisabledonly inspectspolicy-ast, so the convention rules never hold policy parsing open — they run their own pass.types.ts:Finding.categorygains'convention'(renderers/SARIF treat category as an opaque tag, so no switch needs updating).presets.ts:recommendedcarries'C*': 'off'(house style, not a universal fact — the big-tent presets that extend it inherit off);constructiveturns them on (C1 high,C2 medium,C3 low,C4 high).audit.ts: when anyC*rule is enabled, lints every function definition and maps problems toFindings. The function name rides in thetableslot so exposure/overrides/sorting key on it exactly like table findings; suppressed problems becomeacknowledgedfindings withcontext.reason/context.suppressionScope. Function introspection is now memoized so the linter and--call-graphshare one round-trip.An unparseable definition yields no lint findings (opaque bodies are the call-graph's concern, not the linter's).
Test plan
__tests__/lint.test.ts(15 pure unit tests): rule codes/reason policy, C1–C4 detection, all suppression scopes, unparseable input.__tests__/lint-audit.test.ts(6 tests,pgsql-test): C1/C3/C4 fire through the real audit pipeline over a live DB; a reasoned C4 waiver becomes an acknowledged finding with its reason; a clean fully-qualified function yields nothing; preset on/off wiring.presets.test.ts"configures rather than off" invariant to carve outC*(house-style opt-in, mirroring the existingminimalexception).pnpm build,pnpm lint,pnpm testall green (357 tests).Design issue: constructive-io/constructive-planning#1376
Link to Devin session: https://app.devin.ai/sessions/af81a09043504701874ca63e67a9cd4b
Requested by: @pyramation