Conversation
…829) * fix(gcp/computeengine): stamp PaymentOption="monthly" (closes #718) GCP CUDs are billed monthly; there is no upfront billing tier. The "upfront" literal was leftover from AWS-style modelling. Peer services (cloudsql, memorystore, cloudstorage) already emit "monthly". This makes computeengine consistent with them and with ValidPaymentOptionsByProvider["gcp"] = {"monthly"}, silencing the NormalizePaymentOption WARN that fired on every healthy GCP rec. Also adds a PaymentOption assertion to TestComputeEngineClient_ConvertGCPRecommendation to pin the contract. * test(gcp/computeengine): fix godot, misspell, fieldalignment in client_test.go Add period to mock type doc comments (godot); fix British-spelling misspellings in test comments (behaviour->behavior, cancelled->canceled, unrecognised->unrecognized); reorder mock struct fields for optimal alignment (govet/fieldalignment); remove unused index field from MockCommitmentsService. String literal in assert.Contains that matches the production error spelling is nolint-suppressed. * fix(lint): correct spelling of "unrecognised" to "unrecognized" in GCP CUD client US-spelling fix across production error messages and comments in computeengine client; update matching test assertion to "unrecognized". Removes misspell nolint that was suppressing the lint warning.
…chase Term field (#1258) * fix(plans): block past dates in Add Purchases start-date picker (closes #1249) Set `startDateInput.min` to today's ISO date after the default value (tomorrow) is assigned so the browser date-picker rejects past dates while still allowing today as a valid same-day start. Regression test added in plans-range-validation.test.ts: asserts `input.min === today ISO` after `openAddPurchasesModal` returns (FAIL pre-fix, PASS post-fix, 44 tests green). * fix(plans): set Add Purchases start-date min/value via local calendar day `new Date().toISOString().split('T')[0]` returns the UTC calendar date, not the user's local one. For any user west of UTC after their local-evening crossover, the prior `min = toISOString().split('T')[0]` would resolve to tomorrow's UTC date and grey out today in the picker, directly contradicting the PR's own claim that "today remains selectable (a same-day start is legitimate)" (QA 5.6). Extract a `toLocalDateInputValue(Date) -> YYYY-MM-DD` helper that builds the ISO string from local year/month/day components (mirrors the local-midnight pattern already used by `isPlanOverdue` above), and use it for both `value` (default tomorrow) and `min` (today) in `openAddPurchasesModal`. Regression test updated in `plans-range-validation.test.ts`: the prior assertion used the same UTC function as production code, so it would have gone green even with the bug present. The replacement asserts both `min` and `value` against the local-component derivation, and adds an explicit "local calendar day, not UTC" case that documents the failure shape.
… y-axis ticks (#1253) * refactor(chart): extract formatTrendAxisTick to shared chart-utils module Move formatTrendAxisTick from dashboard.ts into frontend/src/modules/chart-utils.ts so it can be reused by the Purchases Savings History chart. Re-export from dashboard.ts for backward compatibility with existing tests and callers. * fix(purchases): correct Savings History chart axis, tooltip decimals, y-axis ticks QA 2.2 -- x-axis starts at period start, not first data point: Convert datasets from scalar arrays to {x: timestamp_ms, y: value} objects and switch the x-axis to type:'linear' with min/max anchored to the selected period window. Mirrors the Home dashboard approach from PR #746. Uses the shared formatTrendAxisTick helper from chart-utils.ts for tick labels. QA 2.3 -- Period Savings tooltip precision matches Cumulative: Change toFixed(4) to toFixed(2) for the Period Savings tooltip label so both series and the KPI box above the chart show 2 decimal places. QA 2.4/2.5 -- y-axis ticks stable when toggling series: Add maxTicksLimit:6 to both y and y1 axis tick configs to cap re-autoscaling on legend toggle. Fix the y1 formatter to emit 2 decimal places for non-integer float ticks, preventing distinct float values from collapsing to the same integer label string. Regression tests added in savings-history.test.ts: - QA 2.2: x-axis type:linear with numeric min equal to period start - QA 2.3: Period Savings tooltip has exactly 2 decimal places - QA 2.4: both y-axes have maxTicksLimit - QA 2.5: y1 formatter does not collapse distinct floats to same label Closes #1252 * refactor(purchases): rename savingsData to periodSavingsData for symmetry Minor readability cleanup: the Period Savings dataset array is now named periodSavingsData, matching its sibling cumulativeSavingsData. No behavior change.
… spelling) (#1277) * fix(db): rename cancelled->canceled (expand-contract, migration 000089) Expand-contract rename of all British-spelled 'cancelled'/'cancellable' variants to US-spelled 'canceled'/'cancelable' across the codebase. - Add migration 000089: adds canceled_by column alongside cancelled_by, widens CHECK constraints to accept both spellings, COALESCE reads both during the expand window (prev version 088) - Rename field CancelledBy->CanceledBy in PurchaseExecution, update json tag to canceled_by; add IsImmediatelyCancelable() predicate - Rename SetCancelledBy->SetCanceledBy in StoreInterface + all implementations and mocks - Update handler_purchases, handler_purchases_revoke and all tests to use US spellings; replace em-dashes with double hyphens in comments - Patch frontend history.ts, riexchange.ts and OpenAPI spec to use 'canceled' status string Rebased onto main (473f69b); migration renumbered from 000082 to 000089 to land after in-flight #808 (000087) and #1428 (000088). * fix(db): restore correct cancel error paths lost in rebase conflict The rebase of 000089 onto current main incorrectly resolved two conflicts in handler_purchases.go: 1. cancelOrRecoverExecution: reverted fmt.Errorf (router->500) back to NewClientError(409,...), misclassifying retriable backend faults as caller faults (feedback_http_status_classification). 2. cancelPurchaseViaSession: took main's broad !IsCancelable() guard instead of the PR's narrower guardImmediatelyCancelable, allowing "scheduled" executions through the pending/notified-only CAS path and producing a misleading "concurrent operation" 409 instead of the clear "use the revoke endpoint" message. Fix: restore fmt.Errorf for the backend-failure branch; add guardCancelableViaSession that explicitly routes "scheduled" to the revoke endpoint before falling through to IsCancelable; rename residual cancelledBy -> canceledBy; remove extra blank line in types.go. Regression tests: TestHandler_deletePlannedPurchase_BackendErrorReturns5xx and TestHandler_cancelPurchase_Session_ScheduledRoutedToRevoke now pass.
…oses #164) (#822) * ux(recommendations): cascading categorical filter distinct values (#164) When building a categorical filter popover for column X, apply all OTHER active column filters to the rec set first. This means selecting Provider=AWS before opening the Service popover now shows only AWS services instead of the full cross-provider list, eliminating ghost values that produce zero rows. Mitigation for the "broaden a column" UX: any value that is part of the column's own currently-active filter is always included in the distinct list even if the cross-filtered set would omit it, so the user can change or deselect the existing value without first clearing every other filter. Algorithm: full re-scan of the cross-filtered rec set on each popover open via the existing applyColumnFilters path. Tests: 2 new assertions -- service popover narrows to AWS-only services when Provider=AWS is active; provider popover still shows all providers when Provider=AWS is its own active filter (own-filter exclusion). * test(recommendations): regression for alwaysInclude in cascading filter popover Add a contradictory-filter test (provider=azure + service=ec2) that verifies values from a column's own active filter remain visible in the popover even when cross-filtering by other columns would omit them. This pins the alwaysInclude behavior added in the cascading-filter implementation: without it, opening the service popover while provider=azure is active would drop ec2 from the list entirely, making it impossible to deselect without first clearing the provider filter. * test(recommendations): assert hostile payload escaping in cascading filter popover Add a regression test that verifies a service value containing an HTML injection string (<script>alert(1)</script>) is stored in dataset and rendered via textContent -- never interpreted as markup -- when the cascading filter popover builds its checkbox list.
…817) secretsmanager:ListSecrets on Resource="*" in SecretsManagerDescribe was a duplicate of the ListSecrets already in the SecretsManager block scoped to cudly-* ARNs. The wildcard grant let the CI/CD role enumerate all secret names in the account. Remove the over-privileged statement.
…) (#818) * sec(ci): gate terraform force-unlock behind explicit input (closes #438) Unconditionally deleting the S3 state lock before every terraform init allowed concurrent dispatch runs to race and corrupt state. Remove the automatic deletion and replace it with a gated step that only runs when the operator explicitly sets clear_stale_lock=true on workflow_dispatch. The existing failure/cancellation cleanup step is unchanged. Add runbooks/terraform-stuck-lock.md with diagnosis and recovery steps. * fix(ci): add language tag to fenced code block in terraform-stuck-lock.md markdownlint MD040 requires fenced code blocks to specify a language. The error-output block at line 7 had a bare ``` opener; add "text" tag. * sec(ci): drop unsafe failure-path state-lock delete (closes #438) The "Release state lock on failure" step deleted the S3 tflock on any failed/cancelled run. A run that fails because it could not acquire the lock would delete the lock a concurrent run is still actively holding, re-introducing the state-corruption race this PR aims to close. Remove the automatic failure-path unlock. Terraform already releases its own lock on a clean apply error; a lock surviving a run means the run died abnormally, which requires operator confirmation via clear_stale_lock before clearing. Update the runbook accordingly.
Align the digest-pin comment block with the production Dockerfile: explain the supply-chain rationale, use `docker buildx imagetools inspect` as the refresh command, reference Renovate/Dependabot, and note the digest must stay in sync with the builder stage.
…872) Replace all 29 caret ranges (^) in package.json with the exact versions already recorded in package-lock.json. Exact pins eliminate the window where `npm install` (vs `npm ci`) resolves a newer minor/patch that may carry unaudited CVEs, and make supply-chain review straightforward. No functional change: every pinned version is the version currently installed, so the lockfile is unchanged.
…int (#915) * chore(iac/aws/networking): remove redundant Secrets Manager VPC endpoint The interface endpoint was unconditional and redundant: every env runs fck-NAT (enable_nat_gateway = true), so in-VPC Secrets Manager consumers (main Lambda, cleanup Lambda, Fargate) will reach SM over NAT regardless. Removes: - aws_vpc_endpoint.secretsmanager (~$14/mo across 2 AZs) - aws_security_group.vpc_endpoints (the endpoint was its sole consumer) - the three matching outputs (vpc_endpoints_security_group_id, secretsmanager_endpoint_id, secretsmanager_endpoint_dns -- grep confirmed no consumers) Header comment updated to reflect actual topology (NAT present, per-service interface endpoints removed). Precondition for safety: enable_nat_gateway must remain true in every environment. If a future change turns NAT off, this endpoint (and likely the ECR ones too) needs to be reintroduced first. * fix(iac/networking): enforce NAT-or-IPv4-endpoint precondition (CR on #915) Add lifecycle.precondition to aws_subnet.private that fails at plan time when enable_nat_gateway is false. Without NAT, module-created private subnets have no IPv4 path to AWS services (Secrets Manager, ECR, etc.) and the module previously planned successfully in this broken configuration. Addresses CodeRabbit Major finding on PR #915. * fix(iac/aws/networking): address CR + pre-commit on SM endpoint removal (#915) - Remove unused data "aws_region" "current" (the SM endpoint was its sole consumer; tflint flagged it as unused). - Restore trailing newline on outputs.tf. - Add lifecycle.precondition on aws_vpc.main that fails the plan when enable_nat_gateway is false: in-VPC workloads (Lambda, Fargate) need NAT for AWS API egress now that per-service interface endpoints are gone. Fails fast instead of relying on doc-only safety as flagged by CodeRabbit. * fix(iac/aws/networking): drop duplicate NAT precondition on aws_vpc.main Commit e395c03 already adds the same precondition on aws_subnet.private, which is the more accurate trigger (it fires only when private subnets are actually being created). Keeping both checked the same variable twice and emitted redundant errors. Drop the one on aws_vpc.main.
The Install Trivy step fetched contrib/install.sh from the mutable main branch of aquasecurity/trivy and piped it straight into sh, so a malicious or accidental change to that script on main would execute arbitrary code on the CI runner, the exact supply-chain weakness the adjacent tflint step already guards against. Fetch the installer from the same pinned release tag as the Trivy binary (v0.69.3, via a TRIVY_VERSION env var so the pin lives in one place), download it to a file before executing it, and use curl -fsSL with set -euo pipefail so transport errors fail loudly instead of feeding an HTML error page into sh. This matches the tflint installer pattern in the same workflow. Verified with actionlint and by running the pinned-tag installer end-to-end with the same arguments (installs trivy 0.69.3). Closes #1187
…#1382) (#1384) * ci: bump gosec pin from v2.26.1 to v2.28.0 The latest securego/gosec release is v2.28.0. Update the self-install in the Security Scanning job so CI runs the current version. PR #1376 (pre-commit gosec hook) is still open; a comment has been posted there asking to align its pin to v2.28.0 when it lands. * ci: retire 10 #nosec annotations obsolete in gosec v2.28.0 Tested each annotation by temporarily removing it and running gosec v2.28.0 on the owning module. Findings that no longer appear indicate the rule was removed or no longer triggers on the pattern. Retired (finding gone in v2.28.0): - G201 x6: postgres_analytics.go (x4) and analytics_postgres.go (x2) -- gosec v2.28.0 removed the G201 SQL-format-string rule entirely - G101 x2: email/templates.go -- gosec no longer flags email copy text containing the word "password" as a hardcoded credential - G101 x1: credentials/resolver.go:27 (azure_client_secret constant) -- gosec no longer flags this string; sibling GCP constants still flagged - G122 x1 (partial): deploy/frontend.go annotation updated from G304,G122 to G304 -- G122 is not a recognized rule in v2.28.0 Kept (finding still returned after removing the annotation): - G302,G304: pkg/common/audit.go -- 0644 file permission + path variable - G404: pkg/retry/exponential.go and providers/aws/recommendations/ratelimiter.go - G115, G101, G704, G703, G304, G204, G117, G505, G706, G104, G703, G705: all remaining annotations (verified per-module) Full verification: gosec v2.28.0 on all 6 modules -> 0 findings; go build ./... and go vet ./... clean; tests pass on all touched packages.
…) (#790) * refactor(frontend/state): add History/ApprovalQueue column-filter slices (refs #166) Adds two new closed column-id enums and matching filter slices for the History page tables: PurchaseHistoryColumnId for the completed-purchases table and ApprovalQueueColumnId for the pending-approvals card. The two tables share Provider/Service/Term/Count/UpfrontCost/MonthlySavings columns but diverge on the queue's Account/Payment/MonthlyCost/CreatedBy vs Purchase History's ResourceType/Region, so each gets its own in-memory slice with set/clear/getAll accessors mirroring the existing recommendations equivalents. In-memory only on this iteration; localStorage persistence stays a follow-up under the same umbrella as the recommendations equivalent. * feat(frontend/history): inline column filters via shared lib — Purchase History table (refs #166) Adds per-column filter buttons to the Purchase History table headers. Filter columns: Provider, Service, Type (resource_type), Region, Term (categorical), and Count, Upfront Cost, Monthly Savings (numeric). Status is excluded — the existing status chip-row is the canonical filter for that column. Wires `applyColumnFilters` from lib/column-filters.ts through extractors that match the rendered cell shape: categorical extractors return the raw field; numeric extractors return the value rounded to display precision so typed numbers match the rendered cells (issue #484 contract). Filters compose with the existing status chip filter, and the column-filter popover lists distinct values from rows that survived the status filter (so picking "Failed" then opening Provider only lists providers with failed rows). Introduces a small lib/history-filter-popover.ts module shared by both History tables — keeps the popover DOM/teardown/keyboard wiring out of history.ts itself and reuses the existing .column-filter-popover CSS so the visual matches the recommendations equivalent. * feat(frontend/history): inline column filters via shared lib — Approval Queue table (refs #166) Adds per-column filter buttons to the Approval Queue table headers. Filter columns: Provider, Account, Service, Term, Payment, Created by (categorical), and Count, Monthly Cost, Upfront Cost, Monthly Savings (numeric). Status is excluded — the queue scope is already pending| notified by definition; the broader Status chip-row above is the authoritative status filter for the page. Uses the same lib/history-filter-popover.ts helper introduced for the Purchase History table. Numeric extractors round to display precision (CURRENCY_DEFAULT_DIGITS) so a "$X" filter matches the rendered cell; monthly_cost returns NaN for null so a "= 0" predicate doesn't match rows where the provider didn't report a value. Categorical extractors mirror the cell rendering: account uses account_id with getAccountName as the display label, created_by prefers email then falls back to UUID. Test-suite mocks (history-* + allowed-accounts + xss-provider-class) extended with the new state accessors so they keep passing against the expanded state surface. * test(frontend/history): column-filter regression suites (refs #166) Adds two test files exercising the new History column-filter wiring: * history-column-filters.test.ts — Purchase History table: numeric expr, categorical set, stacked AND, invalid expr (no-op), clear, and the term-as-stringified-categorical case. * approval-queue-column-filters.test.ts — Approval Queue table: numeric expr (monthly_cost >= N), categorical set (payment in {…}), stacked AND across provider+created_by, invalid expr (no-op), the NaN-as-missing contract for null monthly_cost (so "= 0" and "> 0" don't coincidentally match unreported rows), and clear. Both suites mock the heavy module transitive deps (api / navigation / utils / state / confirmDialog / approval-details / toast / skeleton / recommendations) so the pure column-filter helpers can be exercised without standing up a DOM. * test(frontend/history): add column-filter state mocks to cancel-permissions suite Rebasing onto feat/multicloud-web-frontend surfaced a gap: the new History/ApprovalQueue per-column-filter accessors added to ../state by this PR (issue #166) were missing from the ../state mock in history-cancel-permissions.test.ts. Without them renderApprovalQueue threw "getApprovalQueueColumnFilters is not a function", suppressing the whole approval-queue render and zeroing out the cancel buttons the permission-gating assertions depend on. Add the same six getter/setter/clear mocks already present in the other history test suites so the render path completes and the cancel-gating assertions exercise real button output again. refs #166 * fix(frontend/history): reset trigger aria-expanded on popover close closeOpenHistoryPopover removed the popover DOM but never restored the trigger button's aria-expanded to "false", leaving stale expanded accessibility state after outside-click, Escape, or toggle-close. Reset it on every close path, independent of focus restoration. * sec(frontend/history): escapeHtmlAttr on filter button label+column attrs renderHistoryFilterButton injected `column` and `label` raw into aria-label, title, and data-column attributes via innerHTML template literal. Apply escapeHtmlAttr to both before interpolation and replace the em-dash in the active-state aria-label with a hyphen. Add history-filter-popover-xss.test.ts to assert hostile payloads in label and column are entity-encoded, not executed. * test(frontend/history): add column-filter state mocks to revoke-button suite The #290 revoke-button suite landed on main after this branch added the column-filter state accessors to the other history test suites, so its ../state mock lacked getPurchaseHistoryColumnFilters and the five sibling accessors. After rebasing onto main the render path in history.ts calls them, throws in the mocked suite, and suppresses the whole history-list render, zeroing out the inline Revoke buttons the tests assert on. Add the same six getter/setter/clear mocks already present in the other history suites so the render path completes and the revoke-gating assertions exercise real button output again. refs #166 * test(frontend/history): add History column-filter mocks to marketplace-sell suite loadHistory() now calls state.getPurchaseHistoryColumnFilters() and state.getApprovalQueueColumnFilters() (added by this PR). The history-marketplace-sell-button test mocked state but omitted these new functions, causing the two sell-button-shown assertions to receive [] instead of the expected button IDs. Add the six new state stubs (get/set/clearAll for both slices) matching the pattern already established in the revoke-button and cancel-permissions suites.
* feat(settings): configurable EC2 RI OfferingClass (closes #694) Add a GlobalConfig.OfferingClass field ("convertible" | "standard") that controls which EC2 Reserved Instance offering class is purchased. Empty / absent values default to "convertible" to preserve pre-694 behaviour. Backend: - pkg/common/types.go: OfferingClass field on PurchaseOptions - internal/config/types.go: OfferingClass on GlobalConfig - internal/config/store_postgres.go: read/write offering_class column ($20) - internal/purchase/execution.go: load GlobalConfig and propagate OfferingClass into PurchaseOptions before the fan-out - providers/aws/services/ec2/client.go: resolveOfferingClassType() fails loudly on unknown values (feedback_empty_string_vs_error.md); empty string maps to convertible so the DB default is safe - internal/database/postgres/migrations/000064_ec2_ri_offering_class: ADD COLUMN offering_class TEXT NOT NULL DEFAULT 'convertible' Frontend: - frontend/src/index.html: fieldset with <select> (Convertible / Standard) - frontend/src/settings.ts: load, save, and reset handlers - frontend/src/api/types.ts + types.ts: offering_class optional field Tests: - TestResolveOfferingClassType: unknown values error, "" = convertible - TestDescribeInputFromQuery_OfferingClass: empty / explicit convertible / standard - All findOfferingID call sites updated to 4-arg signature - store_postgres_pgxmock_test.go: +offering_class column in mock rows - settings.test.ts: offering_class: 'convertible' in saveGlobalSettings expected payload * fix(settings): validate OfferingClass on PUT + assert it reaches the EC2 SDK call (refs #694) - Add validateOfferingClass() to GlobalConfig.Validate() so an invalid offering_class is rejected at PUT time with a clear error rather than silently persisting and only failing at purchase time. Accepts "" | "convertible" | "standard" (case-sensitive, matching resolveOfferingClassType). - Add ValidOfferingClasses exported var for reference/documentation. - Add six table-driven test cases in TestGlobalConfig_Validate covering empty (valid), both valid values, wrong-case "Convertible", "STANDARD", and an unknown value. - Add capturingMockEC2Client embedding MockEC2Client that records the last DescribeReservedInstancesOfferingsInput to enable SDK-level assertions. - Add TestFindOfferingID_OfferingClassReachesSDKCall: three subtests ("", "convertible", "standard") each calling findOfferingID and asserting that the captured OfferingClass on the outbound SDK call equals the expected types.OfferingClassType. This test fails if the wiring from offeringClassStr through resolveOfferingClassType to describeInputFromQuery regresses. * refactor(config): extract validateScheduleAndNotifications to reduce cyclomatic complexity Validate had complexity 11 after adding the OfferingClass check in #694. Extract the CollectionSchedule + NotificationDaysBefore + GracePeriodDays checks into validateScheduleAndNotifications, keeping Validate under 10. * fix(test): avoid mutex copy in TestFindOfferingID_OfferingClassReachesSDKCall Construct capturingMockEC2Client directly instead of copying *MockEC2Client to silence the go vet copylocks warning (MockEC2Client embeds mock.Mock which contains sync.Mutex). * fix(purchase): fail EC2 RI recs on GlobalConfig load error instead of defaulting OfferingClass When GetGlobalConfig returns an error, the old code logged it and continued with an empty OfferingClass, which resolveOfferingClassType maps to "convertible". This caused a transient DB failure to silently buy the wrong (and more expensive, irreversible) RI class instead of the operator-configured "standard". Violates the no-silent-fallbacks-on-money-paths rule. Fix: change processPurchaseRecommendations to return (float64, float64, []string, error) and propagate a hard error on GetGlobalConfig failure. Both call sites (executeSingleAccount and executeForAccount) check the new error return and abort without making any cloud purchase. Regression test (HOLE 1): TestProcessPurchaseRecommendations_GlobalConfigError_FailsInsteadOfDefaulting confirms the function returns an error and never calls the provider factory when GetGlobalConfig fails. Verified red on pre-fix code, green after. Also add TestSaveGlobalConfig_OfferingClassBindsAt21 (HOLE 2): a pgxmock test against the real PostgresStore.SaveGlobalConfig verifying offering_class binds as the 21st positional arg. The hand-maintained testablePostgresStore in store_postgres_mock_test.go omits this field entirely, so no fast test previously guarded the 21-placeholder write query. * refactor(purchase): extract applyAccountOutcome to drop executeForAccount below gocyclo 10 The gocyclo pre-commit hook (-over 10) failed: executeForAccount was at cyclomatic complexity 11. Extract the per-account status-resolution switch (partially_completed / failed / completed stamping, #642) and the committed gate (#1014) into a cohesive applyAccountOutcome helper, dropping the function below the threshold without a //nolint or a threshold bump. Behavior is unchanged: same status transitions, same error-note appending, same committed semantics (anyRecPurchased is now evaluated once and reused for the partial flag). The OfferingClass typed-enum validation and no-silent-fallback behavior are untouched. * fix(test): update SQL parameter positions after laddering_enabled shift Rebasing onto main added laddering_enabled as $21 in global_config INSERT, shifting offering_class from $21 to $22. Update the pgxmock and coverage tests to reflect the new column count and binding positions. * style(purchase,config): clear new-from-rev lint on OfferingClass diff Resolve the three golangci-lint findings that --new-from-rev=origin/main attributes to the #694 OfferingClass changes: - errcheck: replace the silent `_ =` discard of SavePurchaseExecution on the processPurchaseRecommendations error path with the existing saveExecutionStatusBestEffort helper, which persists and logs the audit-save failure instead of dropping it. - gocritic unnamedResult: name the four results of processPurchaseRecommendations (its return set grew to include error); switch the trailing assignment from := to = accordingly. - misspell: pre-694 behaviour -> behavior in the ValidOfferingClasses doc. No behavior change to the purchase path; base-debt Lint/Security failures are pre-existing on main and out of scope. * chore(migration): renumber ec2_ri_offering_class from 000078 to 000083 Migration 000078 collided with PR #828. Main's highest is 000081; #1277 reserves 000082; 000083 is the next free slot for this PR. Renamed via git mv (up + down); no Go code referenced the number directly. * chore(migration): renumber ec2_ri_offering_class from 000083 to 000090 Migration 000083 is now taken by ladder_execution_enabled (merged to main while this PR was in review). Next free slot after 000089_rename_cancelled_to_canceled is 000090. * fix(rebase): restore saveExecutionStatusBestEffort; update param-23 guard test Two issues introduced when rebasing onto main (which added ladder_execution_enabled in parallel): 1. saveExecutionStatusBestEffort helper was dropped during conflict resolution of the applyAccountOutcome refactor commit; restored from the pre-rebase branch. 2. TestSaveGlobalConfig_OfferingClassBindsAt22 expected 22 args; with ladder_execution_enabled now at $22, offering_class moved to $23. Renamed the test to TestSaveGlobalConfig_OfferingClassBindsAt23 and added the $22 AnyArg placeholder.
#1231) * fix(scheduledauth): use hardened HTTP client for JWKS warmup and fetch The scheduled-task OIDC validator issued its JWKS warmup probe via http.DefaultClient and let go-oidc's RemoteKeySet fetch keys over the default transport. The JWKS URL is operator-supplied (SCHEDULED_TASK_OIDC_JWKS_URL), so a misconfigured or compromised value could reach internal/metadata endpoints, bypassing the IMDS-blocking hardening the Azure provider clients already use. Move the hardened client (IMDS blocking, dial/TLS/overall timeouts) from providers/azure/internal/httpclient to the shared pkg/httpclient package, since the Azure-internal package is unimportable across the module boundary; the Azure package now delegates to it so there is a single implementation. The validator constructs this client once in configureOIDC, uses it for the warmup probe, and supplies it to go-oidc via oidc.ClientContext so key fetches and rotation refreshes ride the same hardened transport. Regression tests guard all paths: the constructed client must not be http.DefaultClient and must carry a timeout and dedicated transport; Warmup and token-verification key fetches are run with a guarded http.DefaultClient that fails the test if used (both fail pre-fix); pkg/httpclient gets direct IMDS-blocking coverage (IPv4 + IPv6). Closes #1145 * fix(lint): use NewRequestWithContext in httpclient tests to satisfy noctx * fix(lint): suppress G704 SSRF finding on JWKS warmup httpClient.Do gosec's taint analysis flags v.httpClient.Do(req) because v.jwksURL comes from operator-supplied configuration (SCHEDULED_TASK_OIDC_JWKS_URL). The call is safe: jwksURL is operator config, not request-tainted user input, and v.httpClient is the hardened client from pkg/httpclient that blocks IMDS/link-local endpoints and enforces dial/TLS/overall timeouts. Add #nosec G704 with a full justification naming both guards.
* fix(api/history): gate approval-expiry sweep on Lambda runtime The GET /api/history handler expired stale pending/notified approvals in a best-effort background goroutine spawned right before the response returns. On Lambda the execution environment freezes as soon as the response is out, so the goroutine is unreliably suspended mid-sweep: rows can stay "pending" indefinitely and a thawed goroutine doing DB writes during a later request confuses latency and log attribution. Mirror the SWR cache's isLambda gate (ri_utilization_cache.go) using the same runtime.IsLambda detection helper: on Lambda the sweep (a handful of cheap UPDATEs) runs synchronously before the handler returns; on long-running servers it stays asynchronous so the read response is never blocked on the transitions. Regression tests cover both branches by flipping AWS_LAMBDA_RUNTIME_API via t.Setenv: the Lambda sub-test asserts the transition fired before getHistory returned (fails pre-fix), and the non-Lambda sub-test blocks the transition and asserts getHistory still returns (sweep remains asynchronous). Closes #1170 * docs(api/history): refresh isStaleExecution comment after sweep rename Trailing reference to "the async sweep" was left over from the expireStaleExecutionsAsync -> expireStaleExecutions rename in the prior commit. Now the sweep is sync on Lambda and async on servers, so the comment names both modes explicitly. No behavioural change. * fix(api/history): use _rvc index pattern in expireStaleExecutionsSweep Avoids gocritic rangeValCopy warning (304-byte PurchaseExecution copied on each iteration); consistent with the rest of handler_history.go. * ci: trigger fresh CI run (pre-commit gosec runner timeouts)
…mily/region, fail loud on ambiguity (adversarial-review C1) (#1450) Prior to this fix, an EC2Instance Savings Plan purchase could silently commit to the wrong instance family. The Cost Explorer SavingsPlansDetails struct carries InstanceFamily, Region, and OfferingId per-recommendation, but parseSavingsPlanDetail discarded all three. At purchase time, buildSPOfferingsInput applied only a region filter (from the client's configured region, which can differ from the rec's region), with no instanceFamily filter, so lookupOfferingID returned the lexicographically-smallest offering ID across ALL instance families in the region -- potentially buying c6g when CE recommended m5 on a 1-3 year commitment. Fix: - pkg/common: add InstanceFamily, Region, OfferingID fields to SavingsPlanDetails - parser_sp.go: extract and persist those fields from detail.SavingsPlansDetails for EC2Instance SPs only (Compute/SageMaker/Database remain family-agnostic) - client.go (savingsplans): * findOfferingID: when CE provides OfferingID, use it directly (safest path) * buildSPOfferingsInput: accept instanceFamily + recRegion; apply both as DescribeSavingsPlansOfferings filters for EC2Instance SPs, using the rec's region (not client config region) * new lookupEC2OfferingIDStrict: validates result set spans exactly one family and one region; fails loud (error, no purchase) if ambiguous or empty * Compute/SageMaker/Database SP path unchanged (lookupOfferingID) Regression tests: - TestEC2InstanceSP_ResolvesCorrectFamilyAndRegion: mock only matches when both instanceFamily and region filters are present -- fails pre-fix - TestEC2InstanceSP_MultiFamilyResponseFailsLoud: multi-family response now errors instead of silently picking lex-smallest -- fails pre-fix - TestEC2InstanceSP_MultiRegionResponseFailsLoud: same for multi-region - TestEC2InstanceSP_CEProvidedOfferingIDUsedDirectly: direct OfferingID path - TestParseSavingsPlanDetail_EC2InstanceFieldsCaptured: 4 sub-cases verifying parser captures all three fields -- fails pre-fix
… (follow-up to #808) (#1447) * fix(api/marketplace): convert RI term years->months in resale pricing (follow-up to #808) purchase_history.term is stored in years (1 or 3), confirmed by migration 000007 ("valid terms are 0, 1, or 3 (years)") and execution.go formatting it as "%dyr". The marketplaceList handler was passing row.Term unchanged to computeRemainingMonths (param: termMonths int) and resolveMarketplacePriceSchedule (param: originalTerm, documented as months), silently treating years as months. Impact for a 3-year RI sold 6 months in: pre-fix: remainingMonths = max(1, 3-6) = 1; default price ~= $1,140 (3600 * 1/3 * 0.95) post-fix: remainingMonths = 30; default price ~= $2,850 (3600 * 30/36 * 0.95) A caller-supplied {term_months: 30} schedule was also rejected pre-fix (30 > 1 remaining), preventing sellers from specifying the correct term. Fix: guard row.Term <= 0 (error, not silent fallback), then multiply by 12 at the boundary where years enter the pricing math and pass termMonths to both call sites. Update standardRow() in tests from Term:12 (nonsensical 12 years, accidentally masked the bug) to Term:3. Add TestComputeRemainingMonths unit test and TestMarketplaceList_TermYearsConvertedToMonths end-to-end regression test that fails pre-fix and passes post-fix. * fix(frontend/marketplace): convert RI term years->months in Sell gate and consent modal (follow-up to #808) Mirror of the backend years-as-months fix on the frontend. purchase_history.term is stored in years (1 or 3), but frontend/src/history.ts consumed it as months in two places on the Sell-on-Marketplace path: - canSellOnMarketplace (~line 663): computed remainingMonths = term - elapsedMonths with term in years, so a 3-year RI was treated as 3 months and the Sell button vanished after ~3 months of elapsed time. - Consent/pricing modal (~line 1210): computed the residual with term in years, so the resale price summary shown to the user was ~1/3 of the real value (e.g. a 3yr RI 6 months in showed ~$0/underpriced instead of ~$2,850 on $3,600 upfront). Fix: convert term years->months at the boundary (termYears * 12) before computing remainingMonths/residual in both spots, and guard term <= 0. Correct the existing makeRow() test fixture from term:36 (36 years, nonsensical, masked the bug) to term:3. Add a modal-residual regression test asserting a 3yr RI ~6 months in shows ~30 months remaining and a ~$2,850 list price; it fails pre-fix (the gate hides the button) and passes post-fix. Parallels the backend TestMarketplaceList_TermYearsConvertedToMonths.
… delay-config error, safe reaper retry (#1456) F1 (HIGH - approval bypass): add executableByScheduler gate to both ProcessScheduledPurchases cron sweep and handleExecutePurchase SQS path; pending/notified rows with AutoPurchase=false or source=web are skipped/rejected, requiring explicit token-link approval. Gate is fail-closed: a plan-fetch error counts as ineligible. F2 (HIGH - dead auto-create): GetExecutionByPlanAndDate now wraps ErrNotFound on zero rows; getOrCreateExecution branches on errors.Is(ErrNotFound) and defensively treats (nil, nil) as not-found so the create branch is reliably reachable. Pre-fix the create branch was dead code against the real store. F3 (MED-HIGH - fail-open money): both approveViaToken and approvePurchaseViaSession now return 500 when GetGlobalConfig fails instead of silently executing without the configured free-cancel window. Pre-fix: if cfgErr == nil only gated the delay; an error meant immediate execute. F4 (MED - reaper unsafe retry): reapOne only appends "; safe to retry" when allRecsSafeToRedrive returns true. Azure savings-plans have no server-side idempotency key and must not be retried without review. Regression tests added for all four defects; existing tests updated to reflect the new AutoPurchase gate (set AutoPurchase=true on plans that should execute, update ErrNotFound mocks). gocyclo kept under 10 by extracting processOneExecution and checkAutoExecuteGate helpers.
…key constraints on money paths (adversarial-review follow-ups) (#1454) * fix(api): gate empty-account history rows on ownership + enforce API-key constraints on money paths F1 (adversarial-review, medium/cross-scope PII+financial leak): filterPurchaseHistoryByAllowedAccounts previously passed ALL empty-AccountID rows through to scoped users unconditionally (issue #1032 / #621 fix). Any user with view:purchases could see other users' multi-account in-flight rows, including CreatedByUserEmail (PII) and dollar amounts. Fix: gate the exemption on ownership -- only pass through when p.CreatedByUserID == session.UserID. Preserves the #621/#1032 behavior for each user's own ambient in-flight rows; drops other users' rows silently. F2 (adversarial-review, low/med -- money path): requirePermissionConstraints always called HasPermissionForConstraintsAPI with session.UserID, which re-derives constraints from the owning user's GROUP permissions and ignores the user API key's own Constraints (MaxPurchaseAmount, AccountIDs, Providers, etc.). A CI key capped at $100 could spend up to the user's full group limit. Fix: thread the key's DB ID via Session.UserAPIKeyID (set in requirePermission when authenticating via HasAPIKeyPermissionAPI). requirePermissionConstraints now calls HasAPIKeyPermissionForConstraintsAPI when session.UserAPIKeyID != "", evaluating constraints against the key's effective permissions (intersection of key + user group permissions). Falls closed on any lookup error. New methods: - auth.Service.HasAPIKeyPermissionForConstraintsAPI - AuthServiceInterface.HasAPIKeyPermissionForConstraintsAPI - Session.UserAPIKeyID field (json:"-") HasAPIKeyPermissionAPI now returns (userID, keyID, allowed, err) instead of (userID, allowed, err) to avoid a redundant DB lookup. Regression tests: - TestHandler_getHistory_ScopedUserSeesEmptyAccountRows: updated to set CreatedByUserID so the scoped user's own row still passes - TestHandler_getHistory_ScopedUserCannotSeeOtherUsersEmptyAccountRows: F1 guard (fails pre-fix) - TestHandler_executePurchase_UserAPIKeyConstraintsDenied: F2 guard (fails pre-fix, asserts HasAPIKeyPermissionForConstraintsAPI is called) * fix(api): address CodeRabbit security findings on #1454 C1 (handler.go): extract authorizeAPIKey helper and fail closed when HasAPIKeyPermissionAPI returns an empty userID or keyID despite has=true. Without the guard, an empty keyID produces a bearer-session-shaped Session (UserAPIKeyID="") that bypasses the API-key constraint path in requirePermissionConstraints. Extraction keeps requirePermission below the gocyclo-10 pre-commit threshold. C2 (service_apikeys_api.go + service_apikeys.go): enforce the owner's group constraint limits independently in HasAPIKeyPermissionForConstraintsAPI. ComputeEffectivePermissions retains the key's constraint values (e.g. MaxPurchaseAmount) even when they exceed the owner's group limits; a key capped at $1000 owned by a user capped at $100 could previously authorize a $500 request. The fix fetches ownerAuthCtx once (replacing the implicit GetAuthContext inside ComputeEffectivePermissions) and checks each constraint set against both the key's effective permissions and the owner's group permissions. computeEffectivePermissionsFromAuthCtx is extracted as a pure helper so the single authCtx fetch is shared between both checks. Regression test: TestService_HasAPIKeyPermissionForConstraintsAPI_OwnerCapEnforced confirms the $500 request is denied when owner cap is $100, and a $50 request within both caps is allowed.
…ng effects (#1451) maybeAutoHealDirty previously called Force(N) on any dirty schema_migrations row and let Up() continue at N+1. golang-migrate records version=N, dirty=true BEFORE running N's SQL; every migration here is transactional (TestMigrationsAreTransactional), so an interrupted migration rolls back completely, leaving dirty=true at N with N's effects ABSENT. The old auto-heal marked the never-applied migration as complete and skipped its DDL forever while schema_migrations reported success (the 000074 divergence class: 42703 errors at runtime on columns the version record claimed were applied). There is also a narrow race where N commits but the dirty=false update fails, leaving dirty=true WITH effects present. These two cases are indistinguishable from schema_migrations alone without per-migration effect probes, so the conservative fix is to fail loud and let the operator inspect the actual schema before choosing the recovery target via CUDLY_FORCE_MIGRATION_VERSION (N if effects confirmed, N-1 if not). The app still fail-opens (ensureDB swallows the error) and the CloudWatch alarm fires; no crash-loop. Test changes: - Update the two "self-heals" test cases to expect a loud error with recovery instructions and verify the dirty flag is left intact. - Add regression test "dirty-at-N with N-effects-absent": rolls back the last migration (effects absent), injects dirty=true at head, and asserts RunMigrations errors without Force-clearing the flag. - Correct TestMigrations_FullStackIdempotent docstring: the second RunMigrations call returns ErrNoChange without running any SQL, so it does not verify per-migration idempotency; remove the false claim that it "guards the whole directory" and that auto-heal relied on it.
…da) (#1449) The Fargate task role's SES policy granted ses:SendEmail and ses:SendRawEmail on Resource="*" with no Condition, so a compromised ECS task could spoof mail from any SES-verified identity in the account. Split the single "SendFromAnyVerifiedIdentity" statement into two, to mirror the Lambda module's existing defence-in-depth pattern: - "SendFromCUDlyDomain": ses:SendEmail + ses:SendRawEmail on Resource="*" gated by StringLike { "ses:FromAddress" = "*@${var.email_from_domain}" } - "SESReadOnly": ses:GetAccount + ses:GetEmailIdentity on Resource="*" (read-only; no send risk; no Condition required) var.email_from_domain is already declared in the Fargate module's variables.tf and used in the count guard, so no variable threading is needed. Deployments with email_from_domain="" still receive zero SES permissions. terraform fmt and tflint both pass on the touched module. Found via adversarial security review of IaC in origin/main.
closes #1031) (#1041) * fix(api): current_savings zero for services with no active commitments (closes #1031) summarizeRecommendationsWithCoverage incorrectly wrote CurrentSavings to the coverage-scaled potential amount (same as PotentialSavings). Because getDashboardSummary only overwrites CurrentSavings for services present in the purchase-history result, services with recommendations but no active purchases shipped a non-zero current_savings equal to potential_savings. This was wrong: the frontend renders current_savings as committed/realized savings from active purchase history, so an uncommitted service must ship 0. The partial fix in #926 (aggregateActiveCommitmentsPerService + purchase- history overwrite) was correct in concept but left the incorrect assignment in summarizeRecommendationsWithCoverage active, masking the bug for services that had no purchases. Fix: remove svc.CurrentSavings += scaled from summarizeRecommendationsWithCoverage. CurrentSavings is getDashboardSummary's responsibility (via the overwrite loop that calls aggregateActiveCommitmentsPerService). The function comment and the three affected test assertions/functions are updated accordingly. Regression test added: TestHandler_getDashboardSummary_CurrentSavingsZeroWhenNoCommitments asserts current_savings: 0 for services with recommendations but no purchases. * fix(api/dashboard): YTD month accuracy, store error log, fresh slice, first-service dedup * fix(test): update CurrentSavingsZeroWhenNoCommitments to mock GetActivePurchaseHistory getDashboardSummary now calls GetActivePurchaseHistory (active-only, uncapped) instead of GetAllPurchaseHistory to aggregate commitment KPIs (see fetchCommitmentPurchases). The regression test was written before this change and still mocked the old method, causing a testify panic on unexpected method call. Replace the GetAllPurchaseHistory mock with GetActivePurchaseHistory using three mock.Anything matchers (asOf, accountUUIDs, accountExternalIDsByProvider), matching the pattern already used in the sibling CurrentSavingsPopulated and CurrentSavingsJSON tests. * lint(api): fix rangeValCopy and godot findings in handler_dashboard.go golangci-lint (gocritic, godot) flagged three issues introduced by the dashboard commits: - Line 179: range-value copy of config.RecommendationRecord (312 bytes) per iteration; switch to index-based _rvc pattern consistent with the rest of handler_dashboard.go. - Line 229: godot: comment block before summarizeRecommendationsWithCoverage ended in ")" not "."; reword to end in period. - Line 703: godot: calculateCurrentCoverage doc comment missing trailing period. No behavioural change.
…ss (closes #271) (#865) * fix(aws/recommendations): per-call RateLimiter to fix -race (closes #271) RateLimiter was a single *RateLimiter field on Client, shared across the 6 concurrent goroutines that GetAllRecommendations fans out via errgroup. Reset/ShouldRetry/GetRetryCount all mutate retryCount without a lock, triggering data races under go test -race. Per feedback_rate_limiter_per_call.md: each goroutine needs its own independent retry budget (not shared throughput), so per-call instantiation is correct over adding a mutex. Replace rateLimiter *RateLimiter with newRateLimiter func() *RateLimiter. Each fetch*WithRetry / fetch*Page function calls rl := c.newRateLimiter() at entry. Tests inject a factory returning a faster limiter for speed. Also fix mockCostExplorerAPI: callCount and riCalls were mutated by concurrent goroutines without synchronisation. Add sync.Mutex guard. * lint(aws/recommendations): fix godot/misspell/errcheck/gocritic in PR-touched files - godot: add missing periods on doc comments in client.go, client_test.go, parser_sp.go - misspell (US locale): catalogue->catalog, behaviour->behavior, amortised->amortized, cancelled->canceled in touched files - errcheck: replace blank _ = g.Wait() with explicit invariant-enforcing panic so the linter sees the error handled - gocritic unlambda: extract normalizeFilters closure to package-level normalizeFilterSet function - gocritic hugeParam: suppress on functions taking RecommendationParams by value (read-only; pointer-cascade is a larger refactor deferred to a follow-up issue) - nolint:gocritic on NewClient (aws.Config by-value per SDK convention) * fix(aws/recs): pointer-ize hugeParam params in NewClient and SP parsers Remove 6 gocritic hugeParam nolint suppressions from providers/aws/recommendations: - NewClient now accepts *aws.Config instead of aws.Config; callers in service_client.go updated to pass &cfg. - getSavingsPlansRecommendations, fetchSPAllPages, parseSavingsPlansRecommendations, parseSavingsPlanDetail, planTypesForParams all accept *common.RecommendationParams instead of the value type; internal callers updated. Tests in client_test.go, parser_sp_test.go, and parser_sp_additional_test.go updated to pass pointer arguments. * fix(aws/recommendations): use per-call RateLimiter in SP coverage/utilization Rebase on main surfaced sp_coverage.go (added on main, not touched by this PR) still calling the removed shared c.rateLimiter field. Convert its fetchSPCoveragePage and fetchSPUtilizationPage to build a fresh RateLimiter per call via c.newRateLimiter(), matching the race-fix already applied to the RI, coverage, utilization and SP-recommendation paths (feedback_rate_limiter_per_call). This removes the last shared mutable RateLimiter access, so go test -race on the recommendations package is clean. Update the now-obsolete concurrency doc comments on GetSPCoverageSummary and GetSPUtilization, and add the two GetSavingsPlans* methods main added to the CostExplorerAPI interface to mockCostExplorerClient so the aws package test suite compiles. * fix(aws/recs): apply per-call rateLimiter to ondemand_series and fix ladder caller Post-rebase fix: ondemand_series.go was added to main after this PR was cut, still using c.rateLimiter (removed by this PR). Convert to c.newRateLimiter() per-call pattern matching coverage.go/utilization.go. Also update ladder/factory.go to pass &awsCfg to recommendations.NewClient which now takes *aws.Config after the pointer-ize commit in this PR. * style(aws): gofmt service_client_test.go Remove a stray blank line introduced in dfa7a01 that left the file non-gofmt-clean and failed the pre-commit gofmt hook. The file now matches origin/main exactly; no behavioural change.
…ey parsing + nil covered-cost (adversarial-review) (#1455) H1: Azure Search GetRecommendations now returns empty immediately; "AzureSearch" is not a valid Consumption API resourceType. Pre-fix queried with scope-only filter, causing Azure to return VirtualMachine recs mislabeled as Search recs. H2: Replace three hand-written Azure Consumption API resourceType strings with package-level typed constants: - Synapse: "SQLDatabaseDTU" -> reservationResourceTypeSynapse = "SqlDataWarehouse" - SQL DB: "SqlDatabase" -> reservationResourceTypeSQLDB = "SQLDatabases" - CosmosDB:"CosmosDb" -> reservationResourceTypeCosmosDB = "CosmosDB" All three pre-fix strings are invalid enum members; the API returned nothing. M1: ExpandPaymentVariants sets RecurringMonthlyCost to nil (not &0.0) when CommitmentCost is 0, distinguishing "data absent" from "known-zero cost" so the frontend renders "-" instead of fabricating "$0". M2: parseOptionalFloat now returns (float64, error) for money fields (HourlyCommitmentToPurchase, EstimatedMonthlySavingsAmount, UpfrontCost). Present-but-unparseable money fields propagate as errors and drop the recommendation; non-money fields (utilization percentages) still warn+0. Mirrors the RI path (parseAWSCostDetails) which already errored on this class. Extract spPlanTypeDisplayString helper to keep parseSavingsPlanDetail below the gocyclo-10 pre-commit threshold. Regression tests added for all four defects; all fail on pre-fix code and pass on post-fix code.
…ter (#1453) All cancel write paths (CancelExecutionAtomic, CancelScheduledExecutionAtomic, CancelAllPendingExchanges, CancelPendingExchangesByOrigin) were still emitting status='cancelled' and the old cancelled_by column after PR #1277 added the expand-contract rename. TransitionRIExchangeStatus in handler_ri_exchange.go also used the legacy spelling. If #1278 narrows the CHECK before these writers switch, every cancel path fails with check_violation (SQLSTATE 23514). CleanupOldExecutions filtered only 'cancelled', so rows written by new code with status='canceled' were never terminal-cleaned. Fixes: - Switch all cancel SQL writes to StatusCanceled ("canceled") via $N params - Use canceled_by column in CancelExecutionAtomic + CancelScheduledExecutionAtomic - Add 'canceled' to CleanupOldExecutions terminal-status IN clause - Use config.StatusCanceled in rejectRIExchange handler (line ~1372 + response) - Add 8 pgxmock regression tests asserting canonical spelling per path - Update 4 test mocks that hard-coded "cancelled" as the reject-transition arg
…rent_savings, provider-filter KPIs (adversarial-review follow-ups) (#1452) * fix(api/dashboard): exclude revoked commitments, stop fabricating current_savings, provider-filter KPIs (adversarial-review follow-ups) Defect #2 (HIGH): revoked/refunded commitments were counted as active on every money path. Fix applies the predicate at three layers: - SQL: add `revoked_at IS NULL` to GetActivePurchaseHistory WHERE clause (shared across dashboard KPIs, inventory, and analytics snapshots). - In-memory defense-in-depth: isActiveCommitment now returns false when p.RevokedAt != nil, guarding callers that use GetPurchaseHistoryFiltered. - summarizePurchaseHistory: add revoked branch that increments TotalRevoked and skips dollar totals, with TotalRevoked added to HistorySummary. Defect #3 (HIGH): summarizeRecommendationsWithCoverage was setting CurrentSavings = PotentialSavings in the reducer, fabricating realized savings for services with no active commitments. Remove the spurious `svc.CurrentSavings += scaled` line; CurrentSavings is exclusively owned by the getDashboardSummary loop that overwrites from actual purchase_history data. Update tests that asserted the buggy behavior. Defect #4 (MEDIUM): calculateCommitmentMetrics ignored params["provider"], mixing all-provider KPIs (ActiveCommitments, CommittedMonthly, YTDSavings, CurrentSavings, CurrentCoverage) while PotentialMonthlySavings was already filtered. Add provider parameter to calculateCommitmentMetrics and fetchCommitmentPurchases; filter purchases in-memory after the SQL fetch, mirroring fetchCommitmentRecords' existing pattern. Defect #7 (LOW): fetchCommitmentPurchases swallowed GetActivePurchaseHistory errors silently with only a comment. Now emits logging.Errorf so dashboard tiles show "--" (zeroed KPIs) with a visible log trace rather than fabricated $0 with no signal. Regression tests added that fail pre-fix and pass post-fix: - TestIsActiveCommitment_RevokedReturnsFalse - TestHandler_calculateCommitmentMetrics_RevokedExcluded - TestHandler_calculateCommitmentMetrics_ProviderFilter - TestSummarizeRecommendationsWithCoverage_CurrentSavingsIsZero (renamed) - TestSummarizePurchaseHistory_RevokedExcludedFromKPIs * fix(api/types): correct "cancelled" misspelling to "canceled" in comment
) Fixes review findings HYG-05 and HYG-11: - Remove the cost-estimate (Makefile) and profile-new (Makefile.terraform) targets: they invoke scripts/cost-estimate.sh and scripts/generate-profile.sh, which never existed in repo history, so both targets fail immediately. Drop their help lines and the stale references in docs/DEVELOPMENT.md and terraform/profiles/README.md. - Add ci to .PHONY so a file named "ci" cannot mask the target. - Pin install-dev-tools to the CI versions instead of @latest: golangci-lint v2.10.1 (v2 module path), gosec v2.22.4, gocyclo v0.6.0, golang-migrate v4.19.1. staticcheck has no CI pin and is only used by scripts/security-scan.sh; pinned to v0.7.0. - Point "not installed" hints at make install-dev-tools instead of per-tool @latest go install commands. The docker-compose v1 part of HYG-11 was already fixed on main by 5f45f5c (ci: use docker compose v2 instead of docker-compose v1). Verified with make -n on every touched target in both Makefiles and go list -m on each pinned module version. Closes #1174, Closes #1181
) The Makefile.terraform targets docker-build, docker-skip and frontend-skip passed -var flags as a 4th argument to tf-deploy.sh, which only consumed $1..$3, so the flags were silently discarded and each target ran a plain full terraform apply. The named variables (skip_docker_push, skip_docker_build, enable_frontend_build) are not declared root-module variables in any environment, so the targets were never functional; nothing references them. Remove them and their .PHONY entries instead of plumbing dead variables through three cloud roots. Fix the root cause in scripts/tf-deploy.sh so this argument class can never be silently dropped again: arguments after provider/profile/ action are now forwarded verbatim to terraform (set -u safe on bash 3.2 via the ${arr[@]+...} guard). Also make the usage check reachable: PROVIDER=$1 aborted with "unbound variable" under set -u before the intended usage message could print; use ${1:-}/${2:-} defaults. Verified with a stub terraform binary: extra -var/-target args now appear in the rendered command line, plain invocations are unchanged, and the no-args usage path exits 1 with the usage text. make -n shows the removed targets now fail loudly with "No rule to make target". Closes #1172
…1243) The Build & Test section instructed npm run build / npm test / npm run lint at the repo root, where no package.json exists; the root build is Go. The File Organization section mandated /src, /tests, /config, /examples directories that do not exist in this repo. Replace both sections with the real commands (go build ./... and go test ./... at the root, make build / make test-unit / make lint, npm scripts under frontend/) and the actual directory layout (cmd/, internal/, pkg/, providers/, frontend/, terraform/, docs/, scripts/, tests/). All documented commands verified against the Makefile and frontend/package.json; go build ./... run successfully at the root. No other sections changed. Closes #1179
- omit ambient AWS and Azure credential scope from preview audit records - ignore relative XDG_STATE_HOME and fall back to the default state path
Exercise startup and preview behavior while another process holds the audit lock, including unchanged bytes and successful recovery after release. Cover both shared writer and writability-check operations.
cristim
force-pushed
the
feat/mcp-audit-log
branch
from
September 15, 2026 23:46
64f01fb to
2b03108
Compare
Member
Author
|
Rebased the preserved PR commits onto The audit lock now uses nonblocking acquisition with a five-second retry deadline, including EAGAIN/EWOULDBLOCK and EINTR handling. This bounds acquisition attempts, not arbitrary filesystem I/O or the complete purchase request. Added real interprocess regressions for both shared audit operations and the executable MCP consumer: startup rejects a held lock; preview returns its normal dry-run result with a stderr audit warning; existing bytes remain unchanged; after release, a fresh preview appends exactly one expected record. Tests use synthetic credentials and dry-run requests, not cloud purchases. Native macOS verification on Go1.27.1/darwin-arm64 passed: actual MCP lock regression, 40 related MCP tests, full common-package race suite (136 top-level tests), and whole-root Independent fresh-context-origin Astra reviewed the full 36-file candidate; its committed-SHA review evidence follows separately. Linux and exact Go1.26.6 CI remain required, including actual execution of the new consumer tests. No merge is implied by the local results. @coderabbitai full review |
Member
Author
PR1889 independent final-HEAD reviewDate: 2026-09-16. Reviewer: gpt-6-astra, independent from author and planner. Verdict: NO CONFIRMED FINDINGS. The independent final-HEAD code-review and native verification gate passes for commit Identity and review scopeWorktree: Read the complete 36-file committed delta, production interactions and relevant unchanged callers, not only the two recovery tests. Compared every final blob below with the independently reviewed candidate index before and after native testing: all 36 identical. The worktree is clean; Re-read issue LeanerCloud/cloud-commitments-mcp#7 from GitHub at this gate. Three consecutive clean review passes:
Fresh exact-head native evidenceBoth commands completed with exit status 0 under Environment: Go 1.27.1, darwin/arm64,
Commands, run from the worktree root and its GOTOOLCHAIN=local GOMAXPROCS=2 go test -mod=readonly -race -count=1 -p=2 -timeout=180s ./cmd/cudly-mcp ./mcp ./mcp/tools -run '^(TestAuditLogPathDefaultWhenUnset|TestAuditLogPathIgnoresRelativeXDGStateHome|TestAuditLogEmptyValueDisables|TestAuditLogWhitespaceValueDisables|TestAuditLogPathIsTrimmed|TestAuditLogOverrideWins|TestPreviewWritesSkippedRecord|TestPreviewAuditOmitsAmbientCredentialScopeWhenOverrideOmitted|TestPreviewAuditLowercasesExplicitAzureCredentialScope|TestRequestCredentialScopeRespectsPurchaseMode|TestSuccessfulPurchaseWritesSuccessRecord|TestProviderErrorWritesErrorRecord|TestProviderReportedFailureMapsToErrorStatus|TestTwoPurchasesShareOneRunID|TestUnwritablePathWarnsAndDoesNotChangeResult|TestAccountLevelSavingsPlanRegionCannotForkIdempotencyToken|TestEC2InstanceSavingsPlanStillHonorsRegion|TestResolveClientTrimsCredentialScope|TestAzureSubscriptionCaseCannotForkIdempotencyToken|TestAWSProfileCaseIsPreserved|TestProductionAuditDirectoryOpsRejectsUnrepresentableDescriptor|TestEnsureAuditLogDirectorySyncsEveryPathEdge|TestEnsureAuditLogDirectoryJoinsSyncAndCloseErrors|TestEnsureAuditLogDirectoryRetriesExistingHierarchyAfterSyncFailure|TestEnsureAuditLogDirectoryRejectsExistingNonDirectory|TestEnsureAuditLogDirectorySupportsConcurrentCreators|TestEnsureAuditLogDirectoryCreatesPrivateNestedHierarchy|TestEnsureAuditLogDirectoryPreservesExistingMode|TestEnsureAuditLogWritableCreatesNestedHierarchy|TestEnsureAuditLogDirectoryRejectsUnreadableWalkAncestor|TestEnsureAuditLogWritableRejectsUnreadableWalkAncestor|TestPurchaseAuditRejectsUnreadableWalkAncestorWithoutChangingResult|TestCredentialScopeResolution|TestExecutePurchaseAuditLogging|TestExecutePurchaseContradictoryProviderResultIsFailure|TestNewServerBuildsWithoutError|TestNewServerFailsOnBadAuditPath|TestNewServerRejectsNonRegularAuditPath|TestNewServerSucceedsWhenAuditDisabled|TestEndToEndSearchThenDryRunPurchase|TestMainAuditLockTimeout|TestMainRejectsStdoutAuditLogBeforeProtocolTraffic)$' -v
GOTOOLCHAIN=local GOMAXPROCS=2 go test -mod=readonly -race -count=1 -p=2 -timeout=180s ./common -vTests were safety-reviewed: actual MCP requests are previews with synthetic scope and real-purchase enablement cleared; real-outcome unit cases use fake providers. The existing provider-registration test was not run. No real cloud mutation was requested or performed. Root supplied native whole-root build evidence: exit 0 with Go 1.27.1 on the identical precommit candidate. I read that record; I did not independently repeat the whole-root build. Root also reports all normal commit hooks passed without bypass. Those reports are separate from my independently observed exact-head test results. Baseline proofRevalidated all four hashes in baseline manifest. Compared the entire candidate common directory with Baseline raw log shows both operations killed at their ten-second subprocess deadline, at the operation-execution assertion, with package exit 1. This is genuine old-lock regression sensitivity, not a compiler/setup failure. Baseline execution was established earlier; it was not rerun at this final gate. LimitsNative macOS Go 1.27.1 evidence does not establish Linux or Go 1.26.6 compatibility. The full root test suite was not run by this reviewer. No real-provider purchases, power-loss simulation or EINTR fault injection were performed. The five-second bound covers lock acquisition, not arbitrary filesystem I/O. File locking coordinates cooperating writers; configured audit paths are operator-trusted, not a hostile-filesystem security boundary. Static analysis and deterministic filesystem tests do not prove every possible concurrent external path replacement. Reviewed file/blob manifestAll entries below matched the reviewed candidate and committed clean final worktree. |
Contributor
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '--- Repository identity and pull request metadata ---'
git rev-parse HEAD
git status --short
gh pr view 1889 --repo LeanerCloud/reserved-instances-cli \
--json headRefOid,baseRefOid,baseRefName,mergeStateStatus,title,url
printf '%s\n' '--- Merge base and complete changed-file list ---'
base="$(git merge-base origin/main HEAD)"
printf 'merge-base: %s\n' "$base"
git diff --name-status "$base"...HEAD
git diff --stat "$base"...HEAD
git diff --check "$base"...HEAD
printf '%s\n' '--- Recent commits on the pull request ---'
git log --oneline "$base"..HEAD
printf '%s\n' '--- Changed Go source and test structure ---'
ast-grep outline pkg/common/audit.go pkg/common/audit_lock_flock.go \
pkg/common/audit_interprocess_lock_test.go \
mcp/tools/audit.go mcp/server.go cmd/cudly-mcp/main_test.go \
--items all --type function,method
printf '%s\n' '--- Audit lock and shared audit implementation ---'
cat -n pkg/common/audit_lock_flock.go
cat -n pkg/common/audit.go
cat -n pkg/common/audit_interprocess_lock_test.go
printf '%s\n' '--- MCP startup and audit integration ---'
cat -n mcp/tools/audit.go
cat -n mcp/server.go
cat -n cmd/cudly-mcp/main_test.go
printf '%s\n' '--- Lock call sites, configuration, and errors ---'
rg -n -A10 -B8 '\.(Lock|Unlock)\(\)|lock.*timeout|audit.*lock|AuditLock|Err.*Lock|WriteAuditRecord|CheckAuditLogWritable|EnsureAuditLogWritable|recordPurchaseAudit' \
pkg mcp cmd -g '*.go'
printf '%s\n' '--- Complete diff for audit-lock and related consumer changes ---'
git diff --find-renames --unified=80 "$base"...HEAD -- \
pkg/common/audit.go pkg/common/audit_lock_flock.go \
pkg/common/audit_interprocess_lock_test.go pkg/common/audit_internal_test.go \
mcp/tools/audit.go mcp/tools/audit_test.go mcp/server.go mcp/server_test.go \
cmd/cudly-mcp/main_test.go
printf '%s\n' '--- All remaining changed-source diffs ---'
git diff --find-renames --unified=40 "$base"...HEAD -- \
. ':!pkg/common/audit.go' ':!pkg/common/audit_lock_flock.go' \
':!pkg/common/audit_interprocess_lock_test.go' ':!pkg/common/audit_internal_test.go' \
':!mcp/tools/audit.go' ':!mcp/tools/audit_test.go' ':!mcp/server.go' \
':!mcp/server_test.go' ':!cmd/cudly-mcp/main_test.go' \
| sed -n '1,2200p'Length of output: 117050 🏁 Script executed: #!/bin/bash
set -euo pipefail
log=/tmp/coderabbit-shell-logs/shell-output-kISoO2
printf '%s\n' '--- Identity, metadata, and changed files from prior inspection ---'
sed -n '1,220p' "$log"
printf '%s\n' '--- Lock implementation and complete shared audit transaction ---'
cat -n pkg/common/audit_lock_flock.go
cat -n pkg/common/audit.go
cat -n pkg/common/audit_parent_unix.go
printf '%s\n' '--- Exact interprocess lock test functions and supporting constants ---'
rg -n -A140 -B15 'Test(WriteAuditRecord|CheckAuditLogWritable).*Lock|TestAuditLockHelperProcess|auditLockHelper|timed out acquiring audit lock|auditLockTimeout' \
pkg/common/audit_interprocess_lock_test.go pkg/common/audit_internal_test.go
printf '%s\n' '--- Exact MCP held-lock consumer test functions and startup path ---'
rg -n -A180 -B15 'TestMainAuditLockTimeout|audit lock|AuditLock|held lock|lock timeout|CUDLY_MCP_AUDIT_LOG|NewServer' \
cmd/cudly-mcp/main_test.go mcp/server.go mcp/server_test.go mcp/tools/audit.go mcp/tools/audit_test.go
printf '%s\n' '--- Platform targets and build constraints ---'
rg -n -A6 -B4 'runs-on:|GOOS|linux|darwin|windows|container' .github README.md mcp pkg \
-g '*.yml' -g '*.yaml' -g '*.md' -g '*.go' || true
fd -e go . | xargs -r rg -n -A3 -B2 '^//go:build'
printf '%s\n' '--- Changed files and diff summary from prior inspection ---'
rg -n -A180 -B2 -- '--- Merge base and complete changed-file list ---' "$log" | sed -n '1,230p'Length of output: 205199
I found no actionable findings. The audit lock now uses nonblocking The shared audit operations close the file after a lock failure. They preserve the existing audit bytes when acquisition times out. The interprocess tests cover both The MCP consumer test covers both required paths:
The audit writer still validates regular-file targets, serializes appends, repairs incomplete JSONL tails, synchronizes writes, and reports unlock and close errors. GitHub currently reports this PR as I used the existing repository scope guidance: Linux is the supported CI and production target, while Darwin supports development-host verification. The 🧠 Learnings used✅ Action performedFull review finished. |
Member
Author
|
Publication/local gates are complete at 2b03108; this is not merge readiness. Azure Sanity run35037134858 fails at OIDC login because GitHub now presents the immutable renamed-repository subject, while the inspected deploy application still trusts the old CUDly subject. Tracked separately in LeanerCloud/cloud-commitments-platform#336. No Azure trust, permissions or checks were changed. The privileged bootstrap correction needs operator approval; CI and CodeRabbit monitoring continue, and no normal merge will occur until every gate passes. |
Member
Author
|
This PR predates the 2026-09-27 monorepo split and can no longer land as-is: it was branched off the old monorepo history, so its diff spans directories ( The feature has been ported to the split repos, preserving this PR's already-reviewed content (all 14 CodeRabbit review threads resolved, plus the independent
All three are open, CI-green (or in progress), labels mirrored from cloud-commitments-mcp#7, not yet merged pending review and the stated merge order (1 -> 2 and 3). Closing this PR in favor of the three above. Leaving |
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
1 participant
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.
Closes LeanerCloud/cloud-commitments-mcp#7
Why
The MCP server executes real commitment purchases and leaves no durable record.
mcp/README.mdsaid so itself:Those stderr lines vanish with the client session. A money path with nothing to reconcile against is the gap worth closing before adding more purchase surface.
Behaviour
Per
docs/plans/mcp/00-scope.md§4 R1 and §6 Q4:$XDG_STATE_HOME/cudly/mcp-audit.jsonl, falling back to~/.local/state/cudly/mcp-audit.jsonlwhenXDG_STATE_HOMEis unset, empty, or relative. Audit-by-default is the right posture for a tool that spends money.CUDLY_MCP_AUDIT_LOGoverrides the path. Set to the empty string it disables the log — the explicit opt-out. Unset is not the same thing and still uses the default.os.LookupEnvrather thanos.Getenvis what makes that distinction possible, and both branches are tested. A whitespace-only value is treated as the opt-out too, matching how the other operator env vars in this package are read.AuditLogPathis the single resolver. Future reader tools can reuse it instead of re-deriving the configured path.run_id, so every purchase in a server lifetime correlates.status: "skipped",dry_run: true(decision R1). A preview spends nothing but is still a decision worth reconstructing, and the CLI's dry-run path writes the same record.credential_scopepreserves the routing identifier supplied to the purchase: an AWS profile, Azure subscription, or GCP project. It is not mislabeled as a verified provider account ID, and an unsupplied preview scope remains omitted.successonly when the provider returnsSuccess: truewith no embedded error. Dry runs areskipped; provider call errors,Success: false, and results containing an error areerror. Neverskipped_covered. CLI parity is tracked separately in fix(cli): classify errored provider purchase results as failures #1900.NewServerprobes the resolved path at construction and fails on a path it cannot create, rather than silently dropping every record for the session.Nothing here is a tool parameter. The audit log is operator-side configuration; making it model-controllable would let the caller turn off its own audit trail.
stdout is the protocol
cmd/cudly-mcpspeaks MCP over stdio, so any stray stdout write corrupts the transport. Every diagnostic goes through the stdliblogpackage (stderr).git grep 'os.Stdout|fmt.Print|println(' -- mcp/ cmd/cudly-mcp/returns nothing outside tests.ExecutePurchaseis under the repo's gocyclo:10 gate. All three additions are unconditional calls — the status mapping lives inauditStatusFor, and the enabled/disabled and error handling live inrecordPurchaseAudit. gocyclo forExecutePurchaseis 9, unchanged.Verified end to end, not just by tests
The built binary was driven over real stdio with an
initialize/initialized/tools/callsequence againstcudly_aws_ec2_ri_purchasein preview mode withaws_profile=audit-scope-probe:{"run_id":"b944ade1-...","status":"skipped","dry_run":true,"source":"cudly-mcp","service":"ec2","resource_type":"m5.large","count":2,"term_months":12,"provider":"aws","credential_scope":"audit-scope-probe"}Tests
mcp/tools/audit_test.go: path defaults and overrides; explicit empty/whitespace opt-out; preview, success, provider Go error,Success: false, and contradictorySuccess: trueplus embedded error records; exactcredential_scopepersistence; shared processrun_id; and audit-write failure isolation.mcp/server_test.go:NewServerfails on an uncreatable audit path, and succeeds both when auditing is disabled and when the path is writable.Test-isolation hazard handled: with auditing on by default, every existing test calling
ExecutePurchasewould otherwise append to the developer's real~/.local/state/cudly/mcp-audit.jsonl. Both packages'TestMainnow pinCUDLY_MCP_AUDIT_LOGto a run-scoped temp file (mcphad noTestMainbefore). The pre-existing assertion that a preview writes nothing to stderr still holds — a preview's audit record is a file write, not a log line.go build ./...,go test ./mcp/... ./cmd/... ./pkg/...,go vet ./...,gofmt -lall clean.Docs
mcp/README.md: the "No persisted audit record" gap bullet is replaced with an accurate one, plus an## Audit logsection covering the default path, the override, empty-disables, previews-as-skipped, the status mapping, the write-failure posture and the startup probe.Summary by CodeRabbit
New Features
Bug Fixes