Repository navigation
Feat/tool integration v2 - #86
Amazing-Stardom wants to merge 14 commits into
Conversation
![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.
|
|
||
| SET default_table_access_method = heap; | ||
|
|
||
| -- |
There was a problem hiding this comment.
Severity: warning
The _seed_backup table has been removed. Please confirm that it is no longer needed for any processes.
Suggestions:
- Confirm that the
_seed_backuptable is no longer required or its functionality has been safely replaced. - Ensure no data loss or unexpected behavior results from its removal.
| id bigint NOT NULL, | ||
| name text NOT NULL, | ||
| description text NOT NULL, | ||
| lambda_arn text NOT NULL, |
There was a problem hiding this comment.
Severity: info
The Lambda ARN implies a dependency on AWS. This architectural choice should be documented.
Suggestions:
- Add a comment or documentation explaining the dependency on AWS Lambda for tool execution.
| name text NOT NULL, | ||
| description text NOT NULL, | ||
| lambda_arn text NOT NULL, | ||
| multiplier numeric(6,2) DEFAULT 1.0 NOT NULL, |
There was a problem hiding this comment.
Severity: warning
The multiplier column is defined with numeric(6,2) precision. Consider if this precision is sufficient for billing purposes, as double precision is used elsewhere.
Suggestions:
- Ensure
numeric(6,2)provides sufficient precision for billing calculations. Ifdouble precisionis used forcredits_used_monthandcredits_limit_month, consider makingmultiplierconsistent to avoid potential precision issues or unexpected rounding in calculations.
| ALTER SEQUENCE public.loc_usage_ledger_id_seq OWNED BY public.loc_usage_ledger.id; | ||
|
|
||
|
|
||
| -- |
There was a problem hiding this comment.
Severity: critical
The mcp_authorizations table has been removed. Ensure that all code references to this table have been updated accordingly.
Suggestions:
- Verify that all application code, services, and tests that previously interacted with
mcp_authorizationshave been updated or removed. Missing references will cause runtime errors.
| id bigint NOT NULL, | ||
| org_id bigint NOT NULL, | ||
| credits_used_month double precision DEFAULT 0.0 NOT NULL, | ||
| credits_limit_month double precision DEFAULT 50000.0 NOT NULL, |
There was a problem hiding this comment.
Severity: warning
The default value for credits_limit_month is set to 50000.0, which is a magic number. It is recommended to define this as a constant.
Suggestions:
- Replace
50000.0with a named constant or configuration parameter for clarity and easier modification.
| org_id bigint NOT NULL, | ||
| credits_used_month double precision DEFAULT 0.0 NOT NULL, | ||
| credits_limit_month double precision DEFAULT 50000.0 NOT NULL, | ||
| billing_period_start timestamp with time zone NOT NULL, |
There was a problem hiding this comment.
Severity: warning
Default values are missing for the billing_period_start and billing_period_end columns.
Suggestions:
- Add a default value for
billing_period_startandbilling_period_end(e.g.,now()andnow() + interval '1 month') or ensure these are always explicitly set on insert.
| billing_period_end timestamp with time zone NOT NULL, | ||
| created_at timestamp with time zone DEFAULT now() NOT NULL, | ||
| updated_at timestamp with time zone DEFAULT now() NOT NULL, | ||
| CONSTRAINT chk_tool_billing_period_valid CHECK ((billing_period_end > billing_period_start)), |
There was a problem hiding this comment.
Severity: info
The constraint chk_tool_billing_period_valid ensures that the end date is greater than the start date, but it does not enforce that the end date is greater than or equal to the start date.
Suggestions:
- Consider if
billing_period_end >= billing_period_startis more appropriate, allowing for periods of zero duration if needed. If not, this is fine.
| org_id bigint NOT NULL, | ||
| review_id bigint, | ||
| credits_deducted double precision NOT NULL, | ||
| idempotency_key character varying(255) NOT NULL, |
There was a problem hiding this comment.
Severity: critical
The idempotency_key is critical for billing operations. Ensure that a uniqueness constraint is enforced for this key.
Suggestions:
- Add a
UNIQUEconstraint onidempotency_keyto prevent duplicate credit deductions, which is crucial for billing accuracy.
| ADD CONSTRAINT loc_usage_ledger_pkey PRIMARY KEY (id); | ||
|
|
||
|
|
||
| -- |
There was a problem hiding this comment.
Severity: critical
The primary key constraint mcp_authorizations_pkey has been removed. Verify that there are no lingering references to this constraint.
Suggestions:
- Confirm that all code, including foreign key constraints in other tables, no longer references the
mcp_authorizations_pkey.
| operationId: UpdateUser | ||
| parameters: | ||
| - in: path | ||
| name: org_id |
There was a problem hiding this comment.
Severity: critical
The path parameter names for org_id and user_id have been swapped. This represents a breaking change if clients rely on the original order of these parameters.
Suggestions:
- Confirm this change aligns with the actual API endpoint path. If the API path itself has not changed, this is a breaking change for clients using the old parameter order. Communicate this breaking change to API consumers.
| schema: | ||
| type: string | ||
| - in: path | ||
| name: user_id |
There was a problem hiding this comment.
Severity: critical
The path parameter names for org_id and user_id have been swapped. This represents a breaking change if clients rely on the original order of these parameters.
Suggestions:
- Confirm this change aligns with the actual API endpoint path. If the API path itself has not changed, this is a breaking change for clients using the old parameter order. Communicate this breaking change to API consumers.
|
|
||
| ### Tool catalog reference | ||
|
|
||
| The table below lists available tools. `multiplier` is computed by the API (`(memory_mb / 1024) × timeout_s` relative to the cheapest tool) and returned in the `GET /api/v1/orgs/:org_id/tools` response. Tools marked **Beta** are included in the initial seed. |
There was a problem hiding this comment.
Severity: warning
The multiplier is indicated as computed in this context, but the database schema shows it is stored. Please clarify whether it is computed during seeding/updates or dynamically.
Suggestions:
- Clarify how the
multipliervalue is derived and maintained, especially if the 'cheapest tool' baseline changes.
| 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 placeholder hardcodes us-east-1 and ACCOUNT. This needs to be dynamic to support multi-region and multi-account deployments.
Suggestions:
- Ensure
lambda_arnis configurable or derived dynamically based on environment and account.
| 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
For the multiplier column defined as numeric(6,2), please check if the precision is sufficient for all potential cost calculation scenarios.
Suggestions:
- Confirm
numeric(6,2)precision is adequate for all expected multiplier values, especially for very cheap or expensive tools.
| 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: info
The default value for use_case is an empty string (''). Consider using NULL if no specific use case is applicable, or an enum for better consistency.
Suggestions:
- Evaluate using
NULLforuse_caseif it's truly optional, or an enum for better data integrity and consistency.
| 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.
LiveReview Pre-Commit Check: ran (iter:1, coverage:0%)
LiveReview Pre-Commit Check: ran (iter:3, coverage:90%)
179ad51 to
ffaa037
Compare
LiveReview Pre-Commit Check: ran (iter:57, coverage:92%)
LiveReview Pre-Commit Check: ran (iter:1, coverage:0%)
No description provided.