diff --git a/docs/checks.md b/docs/checks.md index 2de9ffb..ce2b5d5 100644 --- a/docs/checks.md +++ b/docs/checks.md @@ -1186,7 +1186,7 @@ These rules live outside the repository-wide `Change Safety` section in report o | Defensive programming | `defensive.invalid-state-representable` | warn | Booleans or raw status strings can represent impossible state combinations. | | Defensive programming | `defensive.null-assumption` | warn | Nullable boundary values are dereferenced without a nil/null guard. | | Defensive programming | `defensive.integer-overflow` | warn | Arithmetic on count, size, or length input lacks an overflow bound check. | -| Defensive programming | `defensive.sequence-collision-risk` | warn | External ID allocation derives the next value from current count without guarded unique-collision retry. | +| Defensive programming | `defensive.sequence-collision-risk` | warn | External ID allocation derives the next value from current count; bounded unique-collision retry is treated as mitigation, not full resolution. | | Defensive programming | `defensive.bounds-assumption` | warn | Indexed access assumes collection bounds without a nearby length check. | | Defensive programming | `defensive.unsafe-default` | warn | A config/env fallback can fail open or disable a safety control. | | Defensive programming | `defensive.non-exhaustive-branch` | warn | Enum-like state/kind/type branching lacks default or exhaustive handling. | diff --git a/internal/codeguard/checks/quality/quality_defensive.go b/internal/codeguard/checks/quality/quality_defensive.go index 2e93d04..c803f55 100644 --- a/internal/codeguard/checks/quality/quality_defensive.go +++ b/internal/codeguard/checks/quality/quality_defensive.go @@ -25,10 +25,10 @@ const ( ) var ( - indexAccessPattern = regexp.MustCompile(`\b([A-Za-z_][\w$]*(?:\.[A-Za-z_][\w$]*)?)\s*\[\s*(?:0|[A-Za-z_][\w$]*)\s*\]`) + indexAccessPattern = regexp.MustCompile(`\b([A-Za-z_][\w$]*(?:\.[A-Za-z_][\w$]*)?)\s*\[\s*([^\]\n]+)\s*\]`) jsonDecodePattern = regexp.MustCompile(`(?i)(json\.Unmarshal|json\.NewDecoder|JSON\.parse|json\.loads|nlohmann::json::parse|decode_json|parseJson)`) externalCallPattern = regexp.MustCompile(`(?i)(http\.Get|client\.Do|fetch\s*\(|axios\.|requests\.(get|post|put|delete)|curl_easy_perform|httplib::|http_client)`) - resourceReadPattern = regexp.MustCompile(`(?i)(io\.ReadAll|ReadAll|read_to_string|read_to_end|\.read\s*\(|bodyParser|multer|upload|request\.body|r\.Body)`) + resourceReadPattern = regexp.MustCompile(`(?i)(io\.ReadAll|ReadAll|read_to_string|read_to_end|\.read\s*\(|bodyParser|multer|upload|formData\s*\(|request\.body|r\.Body|findMany\s*\(|findFirst\s*\()`) unsafeDefaultPattern = regexp.MustCompile(`(?i)(getenv|process\.env|os\.environ|std::getenv|config).*?(default|fallback|\|\||!=|,\s*['"]).*?(true|false|allow|disable|skip|insecure)`) switchLikePattern = regexp.MustCompile(`(?i)\b(switch|match)\b[^{:\n]*(status|state|kind|type)`) stateAssignmentPattern = regexp.MustCompile(`(?i)(status|state)\s*(?:=|:=|=>)\s*["']?(paid|active|complete|completed|shipped|deleted|approved)["']?`) @@ -40,6 +40,7 @@ var ( resourceNamedCountLimit = regexp.MustCompile(`(?i)\b(?:max|limit|quota|cap)[A-Za-z0-9_]*(?:count|size|length|len|bytes)\b`) sequenceAllocationLine = regexp.MustCompile(`(?i)\b(?:external[_]?id|next[_]?id|sequence|slug|number)\b.*(?:count|max)\s*\+\s*1|(?:count|max)\s*\+\s*1.*\b(?:external[_]?id|next[_]?id|sequence|slug|number)\b`) jsonReaderSchemaCall = regexp.MustCompile(`(?i)\b(?:read|parse|decode)Json[A-Za-z0-9_]*\s*\([^)\n,]+,\s*[A-Za-z_$][\w$]*(?:Schema|Validator|Codec|Parser)\b`) + prismaTakePattern = regexp.MustCompile(`(?is)\b(?:findMany|findFirst|findUnique|query|search)\s*\([^)]*\btake\s*:`) ) func defensiveBoundaryFindings(env support.Context, file string, fn precisionFunction) []core.Finding { @@ -58,9 +59,9 @@ func defensiveBoundaryFindings(env support.Context, file string, fn precisionFun findings = append(findings, precisionWarnFinding(env, defensiveNullAssumptionRuleID, file, line, "nullable boundary value is dereferenced without a nil/null guard", core.ConfidenceMedium)) } - if line, ok := sequenceCollisionRiskLine(fn, loweredBody); ok { + if line, message, confidence, ok := sequenceCollisionRiskLine(file, fn, loweredBody); ok { findings = append(findings, precisionWarnFinding(env, defensiveSequenceCollisionRiskRuleID, file, line, - "external ID allocation derives the next value from current count without guarded unique-collision retry", core.ConfidenceMedium)) + message, confidence)) } if line, ok := integerOverflowLine(file, fn, loweredBody); ok { findings = append(findings, precisionWarnFinding(env, defensiveIntegerOverflowRuleID, file, line, @@ -230,7 +231,7 @@ func firstUseLine(fn precisionFunction, name string) int { } func integerOverflowLine(file string, fn precisionFunction, loweredBody string) (int, bool) { - if isUIRenderArithmeticContext(file, fn, loweredBody) { + if isUIRenderArithmeticContext(file, fn, loweredBody) || isSeedOrScriptSourcePath(file) { return 0, false } if sequenceAllocationArithmetic(loweredBody) || metricStatArithmeticContext(fn, loweredBody) || dateCountFormattingContext(fn, loweredBody) { @@ -248,11 +249,21 @@ func integerOverflowLine(file string, fn precisionFunction, loweredBody string) return 0, false } -func sequenceCollisionRiskLine(fn precisionFunction, loweredBody string) (int, bool) { - if !sequenceAllocationArithmetic(loweredBody) || guardedSequenceCollisionRetry(loweredBody) { - return 0, false +func sequenceCollisionRiskLine(file string, fn precisionFunction, loweredBody string) (int, string, string, bool) { + if isSeedOrScriptSourcePath(file) || !sequenceAllocationArithmetic(loweredBody) { + return 0, "", core.ConfidenceLow, false + } + line := firstSequenceAllocationLine(fn) + if guardedSequenceCollisionRetry(loweredBody) { + return line, + "external ID allocation is protected by bounded unique-collision retry, but count-derived IDs remain architectural debt; prefer a database sequence or transactional allocator", + core.ConfidenceLow, + true } - return firstSequenceAllocationLine(fn), true + return line, + "external ID allocation derives the next value from current count; use a database sequence, UUID, or transactional allocator instead of count-based generation", + core.ConfidenceMedium, + true } func sequenceAllocationArithmetic(loweredBody string) bool { @@ -273,7 +284,7 @@ func firstSequenceAllocationLine(fn precisionFunction) int { } func guardedSequenceCollisionRetry(loweredBody string) bool { - if containsAny(loweredBody, []string{"withexternalidretry", "with_external_id_retry"}) && + if containsAny(loweredBody, []string{"withexternalidretry", "with_external_id_retry", "externalidretry", "external_id_retry"}) && containsAny(loweredBody, []string{"p2002", "unique", "collision", "externalid", "external_id"}) { return true } @@ -296,12 +307,13 @@ func metricStatArithmeticContext(fn precisionFunction, loweredBody string) bool func dateCountFormattingContext(fn precisionFunction, loweredBody string) bool { loweredName := strings.ToLower(fn.Name) - if !containsAny(loweredName, []string{"format", "display", "label", "render", "summary", "calendar", "date", "time"}) { + if !containsAny(loweredName, []string{"format", "display", "label", "render", "summary", "calendar", "date", "time", "bucket", "group"}) { return false } return containsAny(loweredBody, []string{ "date", "time", "calendar", "duration", "intl.", "datetimeformat", "formatdistance", - "formatrelative", "plural", "label", "title", "subtitle", "`${", " + \"", " + '", + "formatrelative", "plural", "label", "title", "subtitle", "bucket", "startof", "endof", + "adddays", "subdays", "dayjs", "date-fns", "`${", " + \"", " + '", }) } @@ -321,6 +333,24 @@ func isUIRenderArithmeticContext(file string, fn precisionFunction, loweredBody }) } +func isSeedOrScriptSourcePath(file string) bool { + normalized := strings.ToLower(strings.ReplaceAll(file, "\\", "/")) + base := normalized + if slash := strings.LastIndex(base, "/"); slash >= 0 { + base = base[slash+1:] + } + return strings.Contains(normalized, "/scripts/") || + strings.Contains(normalized, "/script/") || + strings.Contains(normalized, "/seed") || + strings.Contains(normalized, "/seeds/") || + strings.Contains(normalized, "/backfill") || + strings.Contains(normalized, "/import") || + strings.HasPrefix(base, "seed") || + strings.HasPrefix(base, "backfill") || + strings.HasPrefix(base, "import") || + strings.HasPrefix(base, "cleanup") +} + func boundsAssumptionLine(fn precisionFunction, loweredBody string) (int, bool) { if containsAny(loweredBody, []string{"len(", ".length", ".size()", "empty()", "bounds", "range", "count >"}) { return 0, false @@ -334,16 +364,38 @@ func boundsAssumptionLine(fn precisionFunction, loweredBody string) (int, bool) if match == nil { continue } - if nearbyBoundsGuard(fn.Statements, idx, match[1]) { + if !indexExpressionLooksSequenceAccess(match[1], match[2], raw) { continue } - if indexAccessPattern.MatchString(raw) { - return statement.Line, true + if nearbyBoundsGuard(fn.Statements, idx, match[1]) { + continue } + return statement.Line, true } return 0, false } +func indexExpressionLooksSequenceAccess(target string, key string, raw string) bool { + loweredTarget := strings.ToLower(strings.TrimSpace(target)) + loweredKey := strings.ToLower(strings.TrimSpace(strings.Trim(key, `"'`))) + loweredRaw := strings.ToLower(raw) + if strings.Contains(loweredTarget, "process.env") || strings.Contains(loweredTarget, "env") || + strings.Contains(loweredTarget, "map") || strings.Contains(loweredTarget, "dict") || strings.Contains(loweredTarget, "lookup") { + return false + } + if containsAny(loweredRaw, []string{"record<", "map<", "dictionary", "dict", "object.", "hasown", " in ", ".has("}) { + return false + } + if regexp.MustCompile(`^\d+$`).MatchString(loweredKey) { + return true + } + if containsAny(loweredKey, []string{"index", "idx", "offset", "position", "pos", "i", "j", "n"}) { + return true + } + return containsAny(loweredTarget, []string{"array", "list", "slice", "items", "rows", "columns", "chars", "parts", "tokens", "segments", "lines", "values"}) && + !containsAny(loweredKey, []string{"name", "key", "id", "type", "status", "field"}) +} + func nearbyBoundsGuard(statements []support.ParsedStatement, idx int, target string) bool { target = strings.ToLower(strings.TrimSpace(strings.Split(target, ".")[0])) if target == "" { @@ -427,17 +479,21 @@ func missingResourceLimitLine(fn precisionFunction, loweredBody string) (int, bo if boundedReadByteLengthCheck(loweredBody) { return 0, false } + if preFormDataContentLengthHelperGuard(loweredBody) { + return 0, false + } return firstPatternLine(fn, resourceReadPattern), true } func resourceLimitProofPattern(loweredBody string) bool { if containsAny(loweredBody, []string{ "limitreader", "maxbytes", "max_bytes", "content-length", "contentlength", - "limit(", "take(", "slice(", ".slice(", "buffer_size", "quota", + "limit(", "take(", "take:", ".take", "slice(", ".slice(", "buffer_size", "quota", + "maxresults", "max_results", "page_size", "pagesize", }) { return true } - return resourceCountGuard.MatchString(loweredBody) || resourceNamedCountLimit.MatchString(loweredBody) + return resourceCountGuard.MatchString(loweredBody) || resourceNamedCountLimit.MatchString(loweredBody) || prismaTakePattern.MatchString(loweredBody) } func uploadValidationHelperPattern(loweredBody string) bool { @@ -452,6 +508,17 @@ func boundedReadByteLengthCheck(loweredBody string) bool { return containsAny(loweredBody, []string{"bytelength", "byte_length", ".length > max", ".length > limit", "buffer.length", "bytes.length"}) } +func preFormDataContentLengthHelperGuard(loweredBody string) bool { + if !strings.Contains(loweredBody, "formdata") { + return false + } + return containsAny(loweredBody, []string{ + "assertcontentlength", "ensurecontentlength", "validatecontentlength", "guardcontentlength", + "assertrequestsize", "ensurerequestsize", "validaterequestsize", "guardrequestsize", + "assertuploadsize", "ensureuploadsize", "validateuploadsize", "guarduploadsize", + }) +} + func invalidStateTransitionLine(fn precisionFunction, loweredBody string) (int, bool) { if !stateAssignmentPattern.MatchString(functionRawBody(fn)) { return 0, false diff --git a/internal/codeguard/checks/quality/quality_errors.go b/internal/codeguard/checks/quality/quality_errors.go index 45485a7..00b9369 100644 --- a/internal/codeguard/checks/quality/quality_errors.go +++ b/internal/codeguard/checks/quality/quality_errors.go @@ -187,6 +187,9 @@ func cleanupIgnoredLine(statements []support.ParsedStatement) (int, bool) { } func partialFailureHiddenLine(fn precisionFunction, loweredBody string) (int, bool) { + if partialFailureSurfacedInResult(loweredBody) { + return 0, false + } if strings.Contains(loweredBody, "allsettled") && !containsAny(loweredBody, []string{"rejected", "throw", "return err", "return error"}) { return fn.StartLine, true } @@ -205,6 +208,24 @@ func partialFailureHiddenLine(fn precisionFunction, loweredBody string) (int, bo return 0, false } +func partialFailureSurfacedInResult(loweredBody string) bool { + if !containsAny(loweredBody, []string{"diagnostic", "diagnostics", "errors", "failures", "warnings"}) { + return false + } + recordsFailure := containsAny(loweredBody, []string{ + "diagnostics.push", "errors.push", "failures.push", "warnings.push", + "append(diagnostics", "append(errors", "append(failures", "append(warnings", + "diagnostics = append", "errors = append", "failures = append", "warnings = append", + }) + if !recordsFailure { + return false + } + return containsAny(loweredBody, []string{ + "return {", "return result", "diagnostics:", "errors:", "failures:", "warnings:", + "diagnostics,", "errors,", "failures,", "warnings,", + }) +} + func fallbackHidesCorruptionLine(fn precisionFunction, loweredBody string) (int, bool) { if !containsAny(loweredBody, []string{"json", "parse", "deserialize", "unmarshal", "decode", "validate", "corrupt"}) { return 0, false diff --git a/internal/codeguard/checks/quality/quality_precision.go b/internal/codeguard/checks/quality/quality_precision.go index c187db6..20d4c6a 100644 --- a/internal/codeguard/checks/quality/quality_precision.go +++ b/internal/codeguard/checks/quality/quality_precision.go @@ -45,7 +45,7 @@ var ( "misc": {}, "stuff": {}, "value": {}, "values": {}, } queryFunctionPrefixPattern = regexp.MustCompile(`^(get|find|list|load|read|lookup|fetch|is|has|can|should|compute|calculate|build|format|parse)`) - mutatingCallPattern = regexp.MustCompile(`(?i)(^|[.>:\-_])(add|append|assign|clear|create|delete|emit|insert|mutate|persist|pop|publish|push|push_back|remove|reverse|save|send|set|sort|splice|store|update|upsert|write)([A-Z_:\-.]|$)`) + mutatingCallPattern = regexp.MustCompile(`(?i)(^|[.>:\-_])(add|allocate|append|assign|clear|create|delete|emit|insert|mutate|persist|pop|publish|push|push_back|remove|reverse|save|send|set|sort|splice|store|update|upsert|write)([A-Z_:\-.]|$)`) lowLevelOperationPattern = regexp.MustCompile(`(?i)(\bsql\.|\.query\(|\.exec\(|\bhttp\.|\bfetch\(|\baxios\.|\brequests\.|\bjson\.|\bJSON\.|\bos\.Getenv\b|\bprocess\.env\b|\bfs\.|#include\b)`) primitiveTypePattern = regexp.MustCompile(`(?i)\b(string|str|int|int64|float|float64|double|decimal|number|boolean|bool|char|long|short)\b`) domainPrimitiveNamePattern = regexp.MustCompile(`(?i)(id|status|state|type|kind|currency|amount|price|email|phone|country|role|permission|tenant|account|customer|order)`) diff --git a/internal/codeguard/checks/quality/quality_precision_workstreams_cd.go b/internal/codeguard/checks/quality/quality_precision_workstreams_cd.go index 170987e..a82e7e9 100644 --- a/internal/codeguard/checks/quality/quality_precision_workstreams_cd.go +++ b/internal/codeguard/checks/quality/quality_precision_workstreams_cd.go @@ -30,7 +30,7 @@ const ( ) var ( - commandFunctionPrefixPattern = regexp.MustCompile(`^(add|append|assign|cancel|clear|close|create|delete|disable|emit|enable|insert|mutate|notify|open|persist|publish|record|remove|reset|save|send|set|store|submit|toggle|update|upsert|upload|write)`) + commandFunctionPrefixPattern = regexp.MustCompile(`^(add|allocate|append|assign|cancel|clear|close|create|delete|disable|emit|enable|insert|mutate|notify|open|persist|publish|record|remove|reset|save|send|set|store|submit|toggle|update|upsert|upload|write)`) readCallPattern = regexp.MustCompile(`(?i)(^|[.>:\-_])(count|fetch|find|get|list|load|lookup|query|read|select|search)([A-Z_:\-.]|$)`) identifierTokenPattern = regexp.MustCompile(`[A-Za-z_$][A-Za-z0-9_$]*`) infraNamePattern = regexp.MustCompile(`(?i)(sql|http|redis|kafka|grpc|graphql|mongo|s3|dynamo|postgres|mysql|elastic|orm)`) @@ -330,7 +330,7 @@ func explicitMutationName(name string) bool { } func inconsistentReturnContract(fn precisionFunction) bool { - if nextResponseNullableGuardHelper(fn) { + if nextResponseNullableGuardHelper(fn) || nullableParserLookupContract(fn) { return false } returns := returnCategories(fn.Body) @@ -351,6 +351,21 @@ func nextResponseNullableGuardHelper(fn precisionFunction) bool { return strings.Contains(body, "return null") && hasNextResponseBody } +func nullableParserLookupContract(fn precisionFunction) bool { + loweredName := strings.ToLower(strings.Trim(fn.Name, "_$")) + if !regexp.MustCompile(`^(as|exists|extract|find|get|lookup|parse|read|resolve|to)`).MatchString(loweredName) { + return false + } + body := strings.ToLower(fn.Body) + if loweredName == "exists" || strings.HasPrefix(loweredName, "exists") { + return containsAny(body, []string{"return false", "return true"}) + } + signature := strings.ToLower(fn.Signature) + hasNullableReturnEvidence := containsAny(signature, []string{"| null", "|null", "null", "undefined", "optional"}) || + containsAny(body, []string{"return null", "return undefined", "return none", "return nil"}) + return hasNullableReturnEvidence && containsAny(body, []string{"return null", "return undefined", "return none", "return nil"}) +} + type returnShapeCounts struct { total int empty bool @@ -387,7 +402,7 @@ func returnCategories(body string) returnShapeCounts { func isEmptyReturnExpr(expr string) bool { expr = strings.TrimSpace(strings.TrimSuffix(expr, ";")) switch strings.ToLower(expr) { - case "", "nil", "none", "null", "undefined", "false": + case "", "nil", "none", "null", "undefined": return true default: return strings.HasPrefix(expr, "nil,") || strings.HasPrefix(expr, "none,") || strings.HasPrefix(expr, "null,") diff --git a/internal/codeguard/rules/catalog_fix_templates_quality_errors_defensive.go b/internal/codeguard/rules/catalog_fix_templates_quality_errors_defensive.go index 0014fff..0732bb2 100644 --- a/internal/codeguard/rules/catalog_fix_templates_quality_errors_defensive.go +++ b/internal/codeguard/rules/catalog_fix_templates_quality_errors_defensive.go @@ -18,7 +18,7 @@ var qualityErrorDefensiveFixTemplates = map[string]core.FixTemplate{ "defensive.invalid-state-representable": {Kind: guided, Text: "Replace boolean combinations/raw strings with an enum, tagged union, or state machine that encodes valid states."}, "defensive.null-assumption": {Kind: guided, Text: "Guard nil/null/None/optional values before dereference, or make the boundary type non-nullable."}, "defensive.integer-overflow": {Kind: guided, Text: "Guard count/size arithmetic before multiplication, addition, shifts, or allocation sizing."}, - "defensive.sequence-collision-risk": {Kind: guided, Text: "Replace count-plus-one external IDs with database sequences/UUIDs, or wrap allocation in a bounded unique-collision retry."}, + "defensive.sequence-collision-risk": {Kind: guided, Text: "Replace count-plus-one external IDs with database sequences, UUIDs, or a transactional allocator. A bounded P2002/unique-collision retry mitigates collisions but should still be treated as architecture debt."}, "defensive.bounds-assumption": {Kind: guided, Text: "Check length/existence before indexing, or use a safe lookup API."}, "defensive.unsafe-default": {Kind: guided, Text: "Make security/safety defaults fail closed and require explicit opt-out for unsafe behavior."}, "defensive.non-exhaustive-branch": {Kind: guided, Text: "Add an explicit default/unreachable branch or exhaustive assertion for enum-like state switches."}, diff --git a/internal/codeguard/rules/catalog_quality_errors_defensive.go b/internal/codeguard/rules/catalog_quality_errors_defensive.go index 72618f9..e3b7331 100644 --- a/internal/codeguard/rules/catalog_quality_errors_defensive.go +++ b/internal/codeguard/rules/catalog_quality_errors_defensive.go @@ -19,7 +19,7 @@ var qualityErrorDefensiveCatalog = map[string]core.RuleMetadata{ "defensive.invalid-state-representable": localQualityRule("defensive.invalid-state-representable", "warn", "Invalid state representable", "Warns when booleans or raw status strings can represent impossible state combinations.", "Model state with an enum, sum type, or value object that makes invalid combinations unrepresentable."), "defensive.null-assumption": localQualityRule("defensive.null-assumption", "warn", "Null assumption", "Warns when nullable boundary values are dereferenced without a nil/null guard.", "Check for nil/null or validate the value before dereferencing."), "defensive.integer-overflow": localQualityRule("defensive.integer-overflow", "warn", "Integer overflow assumption", "Warns when arithmetic on count, size, or length input lacks an overflow bound check.", "Validate bounds or use a type wide enough for the source range before arithmetic."), - "defensive.sequence-collision-risk": localQualityRule("defensive.sequence-collision-risk", "warn", "Sequence collision risk", "Warns when external ID allocation derives the next value from current count without guarded unique-collision retry.", "Use a database sequence/UUID or handle unique-collision retries explicitly."), + "defensive.sequence-collision-risk": localQualityRule("defensive.sequence-collision-risk", "warn", "Sequence collision risk", "Warns when external ID allocation derives the next value from current count, including retry-mitigated cases that remain architectural debt.", "Prefer a database sequence, UUID, or transactional allocator; bounded unique-collision retry is mitigation, not the target design."), "defensive.bounds-assumption": localQualityRule("defensive.bounds-assumption", "warn", "Bounds assumption", "Warns when indexed access assumes collection bounds without a nearby length check.", "Check length or key existence before indexing."), "defensive.unsafe-default": localQualityRule("defensive.unsafe-default", "warn", "Unsafe default", "Warns when a config/env fallback can fail open or disable a safety control.", "Choose fail-closed defaults and require explicit opt-out for safety-sensitive settings."), "defensive.non-exhaustive-branch": localQualityRule("defensive.non-exhaustive-branch", "warn", "Non-exhaustive branch", "Warns when enum-like state/kind/type branching lacks default or exhaustive handling.", "Handle every known case and add a safe default or exhaustive assertion."), diff --git a/tests/checks/quality_precision_followup_retune_test.go b/tests/checks/quality_precision_followup_retune_test.go new file mode 100644 index 0000000..6277f6d --- /dev/null +++ b/tests/checks/quality_precision_followup_retune_test.go @@ -0,0 +1,233 @@ +package checks_test + +import ( + "fmt" + "path/filepath" + "strings" + "testing" + + "github.com/devr-tools/codeguard/pkg/codeguard" +) + +func TestDefensiveIntegerOverflowFollowupRetunes(t *testing.T) { + dir := t.TempDir() + writeFile(t, filepath.Join(dir, "packages/api/src/routers/external-id-retry.ts"), strings.Join([]string{ + "export async function createEntity(db: Db) {", + " return withExternalIdRetry(async () => {", + " const count = await db.entity.count();", + " const externalId = count + 1;", + " return db.entity.create({ data: { externalId } });", + " }, { code: 'P2002', field: 'externalId' });", + "}", + "declare function withExternalIdRetry(fn: () => Promise, opts: { code: 'P2002'; field: 'externalId' }): Promise;", + "interface Db { entity: { count(): Promise; create(input: unknown): Promise } }", + }, "\n")) + writeFile(t, filepath.Join(dir, "packages/db/scripts/seed-contracts.ts"), strings.Join([]string{ + "export async function seedExternalIds(db: Db) {", + " const count = await db.contract.count();", + " const externalId = count + 1;", + " return db.contract.create({ data: { externalId } });", + "}", + "interface Db { contract: { count(): Promise; create(input: unknown): Promise } }", + }, "\n")) + writeFile(t, filepath.Join(dir, "apps/web/lib/date-buckets.ts"), strings.Join([]string{ + "export function buildDateBuckets(dayCount: number) {", + " const nextBucket = dayCount + 1;", + " return `calendar bucket ${nextBucket}`;", + "}", + }, "\n")) + writeFile(t, filepath.Join(dir, "packages/api/src/lib/unsafe-size.ts"), strings.Join([]string{ + "export function allocateBuffer(count: number, size: number) {", + " const totalBytes = count * size;", + " return new Uint8Array(totalBytes);", + "}", + }, "\n")) + + report := runQualityPrecisionScan(t, qualityPrecisionConfigForLanguage(dir, "typescript")) + + assertFindingRulePresent(t, report, "Code Quality", "defensive.integer-overflow") + assertCodeQualityRuleAbsentForPath(t, report, "defensive.integer-overflow", "external-id-retry.ts") + assertCodeQualityRuleAbsentForPath(t, report, "defensive.integer-overflow", "seed-contracts.ts") + assertCodeQualityRuleAbsentForPath(t, report, "defensive.integer-overflow", "date-buckets.ts") + assertCodeQualityRulePresentForPathWithMessage(t, report, "defensive.sequence-collision-risk", "external-id-retry.ts", "architectural debt", "database sequence", "transactional allocator") + assertCodeQualityRuleAbsentForPath(t, report, "defensive.sequence-collision-risk", "seed-contracts.ts") +} + +func TestAllocateExternalIDIsCommandStyleReturningValue(t *testing.T) { + dir := t.TempDir() + writeFile(t, filepath.Join(dir, "packages/api/src/routers/entity-shared.ts"), strings.Join([]string{ + "export async function allocateExternalId(db: Db) {", + " return withExternalIdRetry(async () => {", + " const count = await db.entity.count();", + " const externalId = count + 1;", + " return db.entity.create({ data: { externalId } });", + " }, { code: 'P2002', field: 'externalId' });", + "}", + "declare function withExternalIdRetry(fn: () => Promise, opts: { code: 'P2002'; field: 'externalId' }): Promise;", + "interface Db { entity: { count(): Promise; create(input: unknown): Promise } }", + }, "\n")) + + report := runQualityPrecisionScan(t, qualityPrecisionConfigForLanguage(dir, "typescript")) + + assertFindingRuleAbsent(t, report, "Code Quality", "function.hidden-mutation") + assertFindingRuleAbsent(t, report, "Code Quality", "function.command-query-mix") + assertCodeQualityRulePresentForPathWithMessage(t, report, "defensive.sequence-collision-risk", "entity-shared.ts", "architectural debt", "database sequence") + assertFindingRuleAbsent(t, report, "Code Quality", "defensive.integer-overflow") +} + +func TestDefensiveBoundsAssumptionDistinguishesDictionaryFromSequenceAccess(t *testing.T) { + dir := t.TempDir() + writeFile(t, filepath.Join(dir, "packages/api/src/lib/field-map.ts"), strings.Join([]string{ + "export function portalFieldLabel(fieldMap: Record, name: string) {", + " return fieldMap[name] ?? 'Unknown';", + "}", + "export function envValue(name: string) {", + " return process.env[name];", + "}", + "export function firstSegment(segments: string[]) {", + " return segments[0];", + "}", + }, "\n")) + + report := runQualityPrecisionScan(t, qualityPrecisionConfigForLanguage(dir, "typescript")) + + assertFindingRulePresent(t, report, "Code Quality", "defensive.bounds-assumption") + assertCodeQualityRuleAbsentForPath(t, report, "defensive.bounds-assumption", "field-map.ts:2") + assertCodeQualityRuleAbsentForPath(t, report, "defensive.bounds-assumption", "field-map.ts:5") +} + +func TestDefensiveResourceLimitCreditsPrismaTakeAndContentLengthHelpers(t *testing.T) { + dir := t.TempDir() + writeFile(t, filepath.Join(dir, "packages/api/src/routers/search-tools.ts"), strings.Join([]string{ + "export async function searchTools(db: Db, query: string) {", + " return db.tool.findMany({ where: { name: { contains: query } }, take: TOOL_SEARCH_LIMIT });", + "}", + "export async function uploadDocument(request: Request) {", + " assertContentLengthWithinLimit(request, MAX_UPLOAD_BYTES);", + " const form = await request.formData();", + " return form;", + "}", + "declare const TOOL_SEARCH_LIMIT: number;", + "declare const MAX_UPLOAD_BYTES: number;", + "declare function assertContentLengthWithinLimit(request: Request, maxBytes: number): void;", + "interface Db { tool: { findMany(input: unknown): Promise } }", + }, "\n")) + writeFile(t, filepath.Join(dir, "packages/api/src/routers/unbounded-upload.ts"), strings.Join([]string{ + "export async function uploadRaw(request: Request) {", + " const form = await request.formData();", + " return form;", + "}", + }, "\n")) + + report := runQualityPrecisionScan(t, qualityPrecisionConfigForLanguage(dir, "typescript")) + + assertFindingRulePresent(t, report, "Code Quality", "defensive.missing-resource-limit") + assertCodeQualityRuleAbsentForPath(t, report, "defensive.missing-resource-limit", "search-tools.ts") +} + +func TestFunctionReturnContractAllowsNullableParserLookupAndExistsHelpers(t *testing.T) { + dir := t.TempDir() + writeFile(t, filepath.Join(dir, "packages/integrations/src/slack/slack-client.ts"), strings.Join([]string{ + "export function parseSlackMessage(value: unknown): SlackMessage | null {", + " if (!isRecord(value)) return null;", + " return { id: String(value.id) };", + "}", + "export function lookupDigestMessage(messages: SlackMessage[], id: string): SlackMessage | null {", + " return messages.find((message) => message.id === id) ?? null;", + "}", + "export function exists(value: unknown) {", + " if (value == null) return false;", + " return true;", + "}", + "interface SlackMessage { id: string }", + "declare function isRecord(value: unknown): value is Record;", + }, "\n")) + + report := runQualityPrecisionScan(t, qualityPrecisionConfigForLanguage(dir, "typescript")) + + assertFindingRuleAbsent(t, report, "Code Quality", "function.inconsistent-return-contract") +} + +func TestErrorPartialFailureHiddenCreditsSurfacedDiagnostics(t *testing.T) { + dir := t.TempDir() + writeFile(t, filepath.Join(dir, "packages/integrations/src/gmail/digest-fetch.ts"), strings.Join([]string{ + "export async function fetchDigest(client: Client, ids: string[]) {", + " const messages: Message[] = [];", + " const diagnostics: string[] = [];", + " for (const id of ids) {", + " try {", + " messages.push(await client.fetch(id));", + " } catch (error) {", + " diagnostics.push(`failed ${id}: ${String(error)}`);", + " continue;", + " }", + " }", + " return { messages, diagnostics };", + "}", + "export async function fetchDigestSilently(client: Client, ids: string[]) {", + " const messages: Message[] = [];", + " for (const id of ids) {", + " try {", + " messages.push(await client.fetch(id));", + " } catch (error) {", + " continue;", + " }", + " }", + " return { messages };", + "}", + "interface Client { fetch(id: string): Promise }", + "interface Message { id: string }", + }, "\n")) + + report := runQualityPrecisionScan(t, qualityPrecisionConfigForLanguage(dir, "typescript")) + + assertFindingRulePresent(t, report, "Code Quality", "error.partial-failure-hidden") + assertCodeQualityRuleAbsentForPath(t, report, "error.partial-failure-hidden", "digest-fetch.ts:1") +} + +func assertCodeQualityRuleAbsentForPath(t *testing.T, report codeguard.Report, ruleID string, pathFragment string) { + t.Helper() + for _, result := range report.Sections { + if result.Name != "Code Quality" { + continue + } + for _, finding := range result.Findings { + location := finding.Path + if finding.Line > 0 { + location = fmt.Sprintf("%s:%d", location, finding.Line) + } + if finding.RuleID == ruleID && strings.Contains(location, pathFragment) { + t.Fatalf("section %q unexpectedly contains rule %q at %s: %s", "Code Quality", ruleID, location, finding.Message) + } + } + return + } + t.Fatalf("section %q not found", "Code Quality") +} + +func assertCodeQualityRulePresentForPathWithMessage(t *testing.T, report codeguard.Report, ruleID string, pathFragment string, messageParts ...string) { + t.Helper() + for _, result := range report.Sections { + if result.Name != "Code Quality" { + continue + } + for _, finding := range result.Findings { + location := finding.Path + if finding.Line > 0 { + location = fmt.Sprintf("%s:%d", location, finding.Line) + } + if finding.RuleID != ruleID || !strings.Contains(location, pathFragment) { + continue + } + loweredMessage := strings.ToLower(finding.Message) + for _, part := range messageParts { + if !strings.Contains(loweredMessage, strings.ToLower(part)) { + t.Fatalf("section %q rule %q at %s message %q does not contain %q", "Code Quality", ruleID, location, finding.Message, part) + } + } + return + } + t.Fatalf("section %q missing rule %q for path containing %q", "Code Quality", ruleID, pathFragment) + } + t.Fatalf("section %q not found", "Code Quality") +} diff --git a/tests/checks/quality_precision_retune_false_positive_test.go b/tests/checks/quality_precision_retune_false_positive_test.go index 26760ad..9521caf 100644 --- a/tests/checks/quality_precision_retune_false_positive_test.go +++ b/tests/checks/quality_precision_retune_false_positive_test.go @@ -262,7 +262,7 @@ func TestDefensiveIntegerArithmeticSplitsSequenceAndMetricContexts(t *testing.T) assertFindingRuleAbsent(t, report, "Code Quality", "defensive.integer-overflow") } -func TestDefensiveSequenceCollisionRecognizesExternalIDRetryHelper(t *testing.T) { +func TestDefensiveSequenceCollisionReportsRetryMitigatedArchitecturalDebt(t *testing.T) { dir := t.TempDir() writeFile(t, filepath.Join(dir, "packages/api/src/external-id-retry.ts"), strings.Join([]string{ "export async function allocateExternalId(db: Db) {", @@ -278,7 +278,7 @@ func TestDefensiveSequenceCollisionRecognizesExternalIDRetryHelper(t *testing.T) report := runQualityPrecisionScan(t, qualityPrecisionConfigForLanguage(dir, "typescript")) - assertFindingRuleAbsent(t, report, "Code Quality", "defensive.sequence-collision-risk") + assertCodeQualityRulePresentForPathWithMessage(t, report, "defensive.sequence-collision-risk", "external-id-retry.ts", "architectural debt", "database sequence") assertFindingRuleAbsent(t, report, "Code Quality", "defensive.integer-overflow") }