Feat/tool integration v2 - #86
Conversation
LiveReview Pre-Commit Check: ran (iter:1, coverage:0%)
LiveReview Pre-Commit Check: ran (iter:2, coverage:0%)
LiveReview Pre-Commit Check: ran (iter:3, coverage:90%)
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Implement Third-Party Tool IntegrationOverviewThis change introduces a new feature enabling third-party static analysis tools to run as Lambda-backed jobs during LiveReview code reviews. It establishes a comprehensive system including new database schemas, API endpoints, UI components, and job orchestration. The system aims to provide detailed tool findings and credit consumption within the review process. Technical Highlights
Impact
|
|
|
||
| 1. THE System SHALL provide an `available_tools` table with columns: `id` (bigserial primary key), `name` (text not null unique), `description` (text not null), and `lambda_arn` (text not null). | ||
| 2. THE System SHALL seed the `available_tools` table with at least two initial rows: one for `ruff` and one for `pylint`, each with a non-empty `description` and a placeholder `lambda_arn`. | ||
| 3. THE System SHALL manage the `available_tools` schema exclusively through dbmate migration files located in `db/migrations/`, and SHALL NOT apply these migrations directly to any production database. |
There was a problem hiding this comment.
Severity: critical
The dbmate migrations have not been applied to the production environment. The strategy for updating the schema needs to be clarified.
Suggestions:
- Clarify how production database schema updates will be managed if dbmate migrations are not applied directly.
- Reconcile this statement with standard dbmate usage for all environments.
|
|
||
| 1. THE System SHALL provide an `org_tools` table with columns: `org_id` (bigint not null, references `organizations.id`), `tool_id` (bigint not null, references `available_tools.id`), `enabled` (boolean not null default false), `config_json` (jsonb not null default `'{}'`), and a composite primary key on (`org_id`, `tool_id`). | ||
| 2. THE System SHALL enforce that every row in `org_tools` references a valid `org_id` in the `organizations` table and a valid `tool_id` in the `available_tools` table via foreign key constraints. | ||
| 3. THE System SHALL manage the `org_tools` schema exclusively through dbmate migration files and SHALL NOT apply these migrations directly to any production database. |
There was a problem hiding this comment.
Severity: critical
The dbmate migrations have not been applied to the production environment. The strategy for updating the schema needs to be clarified.
Suggestions:
- Clarify how production database schema updates will be managed if dbmate migrations are not applied directly.
- Reconcile this statement with standard dbmate usage for all environments.
| 1. WHEN `isCloudMode()` returns `true` AND the authenticated user's role is `owner`, THE Settings Page SHALL render a navigable tab at the hash route `third-party-tools` within `/#/settings`. | ||
| 2. WHEN `isCloudMode()` returns `false`, THE Settings Page SHALL NOT render the `third-party-tools` tab. | ||
| 3. WHEN the authenticated user's role is not `owner`, THE Settings Page SHALL NOT render the `third-party-tools` tab as a clickble navigation item. | ||
| 4. WHEN a non-owner org member navigates directly to `/#/settings#third-party-tools`, THE Settings Page SHALL render a read-only view of the enabled tools without controls to modify them. |
There was a problem hiding this comment.
Severity: warning
A read-only view should be implemented for non-owners. Ensure that no sensitive configuration information is exposed through this view.
Suggestions:
- Verify that the read-only view for non-owners does not expose any sensitive configuration details that should be restricted.
- Implement robust data filtering on the backend for read-only access.
| id bigserial PRIMARY KEY, | ||
| name text NOT NULL UNIQUE, | ||
| description text NOT NULL, | ||
| lambda_arn text NOT NULL, |
There was a problem hiding this comment.
Severity: warning
The lambda_arn column is defined as NOT NULL, but the seeding process requires placeholders. This needs to be reconciled with requirement R1.2.
Suggestions:
- Clarify how
lambda_arnwill be handled for initial seeded tools (R1.2) if they areNOT NULLand 'no hardcoded ARNs belong here'. - Consider allowing
NULLinitially or ensuring a robust placeholder/update mechanism.
| name text NOT NULL UNIQUE, | ||
| description text NOT NULL, | ||
| lambda_arn text NOT NULL, | ||
| multiplier numeric(6,2) NOT NULL DEFAULT 1.0, |
There was a problem hiding this comment.
Severity: warning
A multiplier column has been added. Its purpose is not clear from the schema definition.
Suggestions:
- Add a comment or documentation explaining the purpose and usage of the
multipliercolumn.
| description text NOT NULL, | ||
| lambda_arn text NOT NULL, | ||
| multiplier numeric(6,2) NOT NULL DEFAULT 1.0, | ||
| use_case text NOT NULL DEFAULT '', |
There was a problem hiding this comment.
Severity: warning
A use_case column has been added. Its purpose is not clear from the schema definition.
Suggestions:
- Add a comment or documentation explaining the purpose and usage of the
use_casecolumn.
| created_at timestamptz NOT NULL DEFAULT now() | ||
| ); | ||
|
|
||
| -- Tools are registered via the lr-tools deployer's `register-tools` command, |
There was a problem hiding this comment.
Severity: critical
This comment conflicts with requirement R1.2, which specifies seeding with placeholder ARNs.
Suggestions:
- Reconcile this comment with Requirement 1.2, which states that
available_toolsshould be seeded with placeholderlambda_arns. - Clarify if seeding happens outside of this migration or if placeholders are acceptable here.
| @@ -0,0 +1,15 @@ | |||
| -- migrate:up | |||
| CREATE TABLE IF NOT EXISTS public.org_tools ( | |||
| org_id bigint NOT NULL REFERENCES public.orgs(id) ON DELETE CASCADE, | |||
There was a problem hiding this comment.
Severity: warning
The ON DELETE CASCADE option is used for the orgs table. Please confirm that this aligns with the business logic requirements.
Suggestions:
- Confirm that cascading deletion of
org_toolsentries uponorgsdeletion aligns with business requirements. - Consider
ON DELETE RESTRICTorSET NULLiforg_toolsdata needs to be preserved or handled differently.
| -- migrate:up | ||
| CREATE TABLE IF NOT EXISTS public.org_tools ( | ||
| org_id bigint NOT NULL REFERENCES public.orgs(id) ON DELETE CASCADE, | ||
| tool_id bigint NOT NULL REFERENCES public.available_tools(id) ON DELETE CASCADE, |
There was a problem hiding this comment.
Severity: warning
The ON DELETE CASCADE option is used for the available_tools table. Please confirm that this aligns with the business logic requirements.
Suggestions:
- Confirm that cascading deletion of
org_toolsentries uponavailable_toolsdeletion aligns with business requirements. - This is generally acceptable, as configurations for a non-existent tool are usually irrelevant.
| tool_id bigint NOT NULL REFERENCES public.available_tools(id) ON DELETE CASCADE, | ||
| enabled boolean NOT NULL DEFAULT false, | ||
| config_json jsonb NOT NULL DEFAULT '{}', | ||
| updated_at timestamptz NOT NULL DEFAULT now(), |
There was a problem hiding this comment.
Severity: warning
The updated_at column lacks an ON UPDATE trigger, meaning it is only set upon insertion and not updated thereafter.
Suggestions:
- Add an
ON UPDATEtrigger to automatically update theupdated_attimestamp whenever a row inorg_toolsis modified. - Consider using the
org_tool_billing_state_set_updated_at()function if applicable.
| @@ -1,7 +1,7 @@ | |||
| \restrict dbmate | |||
|
|
|||
| -- Dumped from database version 15.17 (Debian 15.17-1.pgdg13+1) | |||
There was a problem hiding this comment.
Severity: warning
There is a PostgreSQL version bump from 15.17 to 16.14. Please ensure compatibility with existing applications and queries.
Suggestions:
- Verify compatibility of all application components and dependencies with PostgreSQL 16.14.
- Ensure all environments (dev, staging, prod) are aligned or tested for this version.
|
|
||
| -- | ||
| -- Name: scheduled_review_configs scheduled_review_configs_repository_id_fkey; Type: FK CONSTRAINT; Schema: public; Owner: - | ||
| -- Name: scheduled_review_configs scheduled_review_configs_org_id_fkey; Type: FK CONSTRAINT; Schema: public; Owner: - |
There was a problem hiding this comment.
Severity: warning
When performing the foreign key swap for scheduled_review_configs, ensure that the migration correctly handles existing rows.
Suggestions:
- Verify that all existing repository_id values map correctly to org_id via the new FK.
| operationId: UpdateUser | ||
| parameters: | ||
| - in: path | ||
| name: org_id |
There was a problem hiding this comment.
Severity: critical
There is a parameter order mismatch; please check the potential impact on client-side functionality.
Suggestions:
- Audit all API clients for breaking changes due to parameter swap.
- Verify backend route handler matches new parameter order.
| - Procedural sentences contain a maximum of 20 words. | ||
| - Descriptive sentences contain a maximum of 25 words. | ||
| - All verbs use simple present, simple past, or simple future tenses. | ||
| - The text avoids unapproved modal verbs (`should`, `would`, `may`, `might`, `could`). |
There was a problem hiding this comment.
Severity: warning
The unapproved modal verb 'should' is used here, which violates the STE100 rules.
Suggestions:
- Replace 'should' with 'must' or 'will' to comply with Simplified Technical English requirements.
| if callErr != nil { | ||
| return "", 0, 0, callErr | ||
| } | ||
| return text, len(input) / 4, len(text) / 4, nil |
There was a problem hiding this comment.
Severity: warning
The heuristic token estimation, calculated as len/4, is inaccurate and may lead to billing or quota issues.
Suggestions:
- Use a proper tokenizer library (e.g., tiktoken-go) instead of length/4.
- Log a warning when falling back to estimation.
| } else if len(dbComments) > 0 { | ||
| for _, dbC := range dbComments { | ||
| var contentMap map[string]interface{} | ||
| if err := json.Unmarshal(dbC.Content, &contentMap); err != nil { |
There was a problem hiding this comment.
Severity: warning
Unmarshaling comment content inside a loop could become a potential performance bottleneck, especially for large reviews.
Suggestions:
- Consider batch processing or pre-fetching if comment volume is high.
| dbURL = os.Getenv("DATABASE_URL") | ||
| } | ||
| if dbURL == "" { | ||
| dbURL = "postgres://livereview:livereview_password_123@localhost:5432/livereview?sslmode=disable" |
There was a problem hiding this comment.
Severity: warning
Database credentials are hardcoded directly within the test, which is not recommended.
Suggestions:
- Use environment variables exclusively for test DB configuration.
| if err := c.Bind(&req); err != nil { | ||
| return c.JSON(http.StatusBadRequest, map[string]string{"error": "invalid request body"}) | ||
| } | ||
| if req.Name == "" || req.LambdaARN == "" { |
There was a problem hiding this comment.
Severity: warning
Validation for empty strings is missing; consider using a validator package or adding explicit checks.
Suggestions:
- Implement a struct validation library like go-playground/validator
- Add explicit checks for all required fields
|
|
||
| for _, tool := range enabledTools { | ||
| dispatchJSON := fmt.Sprintf(`{"tool_id": %d, "tool_name": %q, "status": "pending"}`, tool.ID, tool.Name) | ||
| if _, execErr := w.db.ExecContext(ctx, ` |
There was a problem hiding this comment.
Severity: warning
Direct SQL execution within a loop is present, which could lead to a potential performance bottleneck.
Suggestions:
- Batch insert review events using a single query
- Use a transaction if multiple inserts are required
| LambdaARN: tool.LambdaARN, | ||
| DiffZipBase64: args.DiffZipBase64, | ||
| } | ||
| if err := w.jq.QueueToolReviewJob(ctx, toolJobArgs); err != nil { |
There was a problem hiding this comment.
Severity: warning
Error handling for failed job queueing is missing, which results in a silent failure.
Suggestions:
- Return error to stop processing if critical
- Implement retry logic or dead-letter queueing
| } | ||
|
|
||
| // NewToolReviewWorker creates a new ToolReviewWorker with AWS config loaded from env. | ||
| func NewToolReviewWorker(db *sql.DB) *ToolReviewWorker { |
There was a problem hiding this comment.
Severity: warning
In NewToolReviewWorker, no context is being passed to LoadDefaultConfig.
Suggestions:
- Pass the provided
context.Contexttoconfig.LoadDefaultConfigfor proper cancellation and tracing.
| accessKey := os.Getenv("AWS_ACCESS_KEY_ID") | ||
| secretKey := os.Getenv("AWS_SECRET_ACCESS_KEY") | ||
| region := os.Getenv("AWS_REGION") | ||
| if region == "" { |
There was a problem hiding this comment.
Severity: warning
The AWS region is hardcoded; consider making it configurable.
Suggestions:
- Make the AWS region configurable via an environment variable or a dedicated configuration struct.
- Ensure
AWS_REGIONis always set in deployment environments.
| } | ||
| if err != nil { | ||
| log.Printf("[WARN] ToolReviewWorker: failed to load AWS config: %v. Creating fallback config.", err) | ||
| awsCfg = aws.Config{Region: region} |
There was a problem hiding this comment.
Severity: warning
The use of a fallback AWS configuration might inadvertently hide critical errors.
Suggestions:
- Re-evaluate if a fallback config is truly desired. If
config.LoadDefaultConfigfails, it often indicates a critical environment setup issue that should halt the worker. - If fallback is necessary, ensure it's well-documented and its limitations are understood.
| _ = eventSink.EmitLogEvent(ctx, args.ReviewID, args.OrgID, "error", fmt.Sprintf("Lambda execution for tool %s failed: %v", args.ToolName, err), "") | ||
| // Write synthetic failure event | ||
| toolsStore := storagetools.NewToolsStore(w.db) | ||
| failJSON := fmt.Sprintf(`{"exit_code": -1, "findings": [], "stderr": %q}`, err.Error()) |
There was a problem hiding this comment.
Severity: warning
The construction of the synthetic failure event JSON carries a potential injection risk.
Suggestions:
- Sanitize
err.Error()before embedding it directly into a JSON string to prevent malformed JSON or unexpected data. - Consider using
json.Marshalfor thefailJSONobject to ensure proper escaping and structure.
| } | ||
|
|
||
| // Parse findings and classify using deterministic rule map + taxonomy | ||
| var altResp struct { |
There was a problem hiding this comment.
Severity: info
A large anonymous struct is used for the Lambda response; it would be better to define a named struct for clarity and maintainability.
Suggestions:
- Extract this anonymous struct into a named type (e.g.,
LambdaToolResponse) for better readability and reusability. - Consider defining sub-structs for
FindingsandStart/Extrafields.
| } `json:"extra"` | ||
| } `json:"findings"` | ||
| } | ||
| if jsonErr := json.Unmarshal(outBytes, &altResp); jsonErr != nil { |
There was a problem hiding this comment.
Severity: warning
An unmarshal error is logged as a WARN, but processing continues, which might lead to unexpected behavior.
Suggestions:
- Clarify if this behavior is intended. If malformed JSON from Lambda should prevent classification, then
return fmt.Errorf(...)here. - If continuing is desired, document why a partial success (raw stored, classification skipped) is acceptable.
| if jsonErr := json.Unmarshal(outBytes, &altResp); jsonErr != nil { | ||
| log.Printf("[WARN] ToolReviewWorker: failed to unmarshal Lambda findings output for tool=%s review=%d: %v", args.ToolName, args.ReviewID, jsonErr) | ||
| } else if len(altResp.Findings) > 0 { | ||
| rawList := make([]toolclassifier.RawToolFinding, len(altResp.Findings)) |
There was a problem hiding this comment.
Severity: warning
The logic for extracting finding fields is complex and appears brittle to future changes.
Suggestions:
- Centralize this logic if multiple tools have similar output variations.
- Consider a more robust mapping or adapter pattern if tool outputs are highly variable.
- Add comments explaining the priority order for field extraction.
| rm := reviewprocessor.NewReviewManager(w.db) | ||
| stored := 0 | ||
| for _, c := range classifiedComments { | ||
| if c == nil { |
There was a problem hiding this comment.
Severity: warning
There is a nil check for a classified comment, but ClassifyToolResult should ideally not return nil elements.
Suggestions:
- Review
toolclassifier.ClassifyToolResultto ensure it does not returnnilelements in the slice. - If
nilelements are possible, add a comment explaining why and if they should be filtered earlier.
| } | ||
|
|
||
| // finalizeIfAllDone checks if all dispatched tools have returned results and marks the review completed. | ||
| func (w *ToolReviewWorker) finalizeIfAllDone(ctx context.Context, args ToolReviewJobArgs) error { |
There was a problem hiding this comment.
Severity: critical
There is a race condition risk because multiple workers can finalize concurrently.
Suggestions:
- Implement a distributed lock (e.g., using
pg_advisory_lock) around thefinalizeIfAllDonelogic for a givenreview_id. - Consider using a transactional approach or a single-threaded finalization mechanism to prevent multiple completion events or status updates.
| // finalizeIfAllDone checks if all dispatched tools have returned results and marks the review completed. | ||
| func (w *ToolReviewWorker) finalizeIfAllDone(ctx context.Context, args ToolReviewJobArgs) error { | ||
| var pendingJobs int | ||
| err := w.db.QueryRowContext(ctx, ` |
There was a problem hiding this comment.
Severity: warning
The SQL query is inefficient due to JSON field extraction being performed within the WHERE or NOT IN clauses.
Suggestions:
- Add a GIN index on the
datacolumn forreview_eventsto speed up JSON key lookups (e.g.,CREATE INDEX idx_review_events_data_tool_name ON public.review_events USING GIN (data jsonb_path_ops);). - Consider restructuring
review_eventsschema to havetool_nameas a direct column if this query is frequent, avoiding JSON extraction overhead.
|
|
||
| eventSink := reviewprocessor.NewDatabaseEventSink(w.db) | ||
| if emitErr := eventSink.EmitCompletionEvent(ctx, args.ReviewID, args.OrgID, | ||
| "### Static Analysis Tools Review Only\n\nAI review skipped due to --tools flag.", |
There was a problem hiding this comment.
Severity: warning
The completion message is hardcoded and may not always be accurate.
Suggestions:
- Make the completion message dynamic based on the actual review type or flags.
- Pass the appropriate message as an argument to
finalizeIfAllDone.
|
|
||
| // IsToolsEligible returns true if the plan type is eligible for static analysis tool credit deduction. | ||
| func IsToolsEligible(p PlanType) bool { | ||
| if p == PlanType("") { |
There was a problem hiding this comment.
Severity: warning
An empty PlanType currently returns true; please clarify the intended behavior.
Suggestions:
- Clarify if an empty
PlanTypeshould indeed be eligible for tools. This might allow tool usage without a valid plan. - If not intended, change
return truetoreturn falseor handlePlanType("")as an invalid state.
| return nil, nil | ||
| } | ||
|
|
||
| var raw map[string]interface{} |
There was a problem hiding this comment.
Severity: warning
Manual type assertions are used for each field, which makes the code verbose and error-prone.
Suggestions:
- Consider using a library like
mapstructurefor safer, more concise mapping frommap[string]interface{}toToolRuleConfig. - Extract field parsing logic into smaller helper functions for
enabled,category,include,exclude.
| results[toolName] = &ToolRuleConfig{Enabled: &bVal} | ||
| case map[string]interface{}: | ||
| cfg := &ToolRuleConfig{} | ||
| if enabledRaw, exists := v["enabled"]; exists && enabledRaw != nil { |
There was a problem hiding this comment.
Severity: info
The repeated exists && enabledRaw != nil checks appear redundant.
Suggestions:
- Simplify:
if enabledVal, ok := v["enabled"].(bool); ok { cfg.Enabled = &enabledVal }. If key doesn't exist or is nil,okwill be false.
| } | ||
| } | ||
| if incRaw, exists := v["include"]; exists && incRaw != nil { | ||
| if incSlice, ok := incRaw.([]interface{}); ok { |
There was a problem hiding this comment.
Severity: warning
Non-string items in the include slice are silently skipped, which could lead to potential configuration misinterpretation.
Suggestions:
- Add logging for non-string items in
includeorexcludeto alert users to malformed configuration. - Consider returning an error if unexpected types are found, making configuration stricter.
| } | ||
| } | ||
| } | ||
| if excRaw, exists := v["exclude"]; exists && excRaw != nil { |
There was a problem hiding this comment.
Severity: warning
Non-string items in the exclude slice are silently skipped, which could lead to potential configuration misinterpretation.
Suggestions:
- Add logging for non-string items in
includeorexcludeto alert users to malformed configuration. - Consider returning an error if unexpected types are found, making configuration stricter.
| return true | ||
| } | ||
|
|
||
| var includeMatcher *gitignore.GitIgnore |
There was a problem hiding this comment.
Severity: warning
The gitignore matcher creation logic is duplicated in both ShouldRunToolRuleForDiff and FilterLocalCodeDiffsForTool.
Suggestions:
- Extract
createGitIgnoreMatchers(cfg *ToolRuleConfig)helper function to return(includeMatcher, excludeMatcher *gitignore.GitIgnore). - Call this helper once at the start of both functions.
| } | ||
|
|
||
| matchingFilesCount := 0 | ||
| for _, d := range diffs { |
There was a problem hiding this comment.
Severity: warning
The path extraction logic is duplicated in both ShouldRunToolRuleForDiff and FilterLocalCodeDiffsForTool.
Suggestions:
- Extract
getDiffPath(d lib.LocalCodeDiff)helper function. - Call this helper within the loop in both functions.
| return diffs | ||
| } | ||
|
|
||
| var includeMatcher *gitignore.GitIgnore |
There was a problem hiding this comment.
Severity: warning
The gitignore matcher creation logic is duplicated in both ShouldRunToolRuleForDiff and FilterLocalCodeDiffsForTool.
Suggestions:
- Extract
createGitIgnoreMatchers(cfg *ToolRuleConfig)helper function to return(includeMatcher, excludeMatcher *gitignore.GitIgnore). - Call this helper once at the start of both functions.
|
|
||
| var filtered []lib.LocalCodeDiff | ||
| for _, d := range diffs { | ||
| path := d.NewPath |
There was a problem hiding this comment.
Severity: warning
The path extraction logic is duplicated in both ShouldRunToolRuleForDiff and FilterLocalCodeDiffsForTool.
Suggestions:
- Extract
getDiffPath(d lib.LocalCodeDiff)helper function. - Call this helper within the loop in both functions.
|
|
||
| b.WriteString(fmt.Sprintf("diff --git a/%s b/%s\n", oldPath, newPath)) | ||
| if d.OldPath == "/dev/null" || d.OldPath == "" { | ||
| b.WriteString("new file mode 100644\n") |
There was a problem hiding this comment.
Severity: warning
The file mode 100644 is hardcoded and may not be accurate for all file types, such as executables.
Suggestions:
- If
lib.LocalCodeDiffcontains file mode information, use that instead. - If not, consider if
100644is always acceptable or if a default is sufficient for current use cases.
5 participants
No description provided.