feat(spp_simulation): add simulation engine for program scenario modeling#46
feat(spp_simulation): add simulation engine for program scenario modeling#46
Conversation
Summary of ChangesHello @jeremi, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request delivers a powerful new Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| _logger.info( | ||
| "Simulation completed for scenario '%s': %d beneficiaries, cost %.2f, " "equity %.0f, duration %.2fs", | ||
| scenario.name, | ||
| beneficiary_count, |
Check failure
Code scanning / CodeQL
Clear-text logging of sensitive information High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 10 hours ago
In general, to fix clear‑text logging of sensitive information, you should avoid writing sensitive values directly into logs. Either (a) remove them from log messages entirely, (b) replace them with more general or coarser-grained information, or (c) mask/obfuscate them so that logs remain useful but do not expose sensitive details.
Here, the minimum-impact fix is to keep the simulation behavior and database writes exactly the same (we still store beneficiary_count in the run record), but change the _logger.info call so that it no longer logs beneficiary_count. We can still log other high-level, presumably non-sensitive metrics (e.g., total cost, equity score, duration) or, if desired, replace the beneficiary count with a generic statement that does not reveal the exact number.
Concretely in spp_simulation/services/simulation_service.py:
- Locate the
_logger.infocall around lines 131–138. - Remove the
%d beneficiariesplaceholder from the format string and the associatedbeneficiary_countargument. - Optionally, adjust the message to continue making sense, e.g.,
"Simulation completed for scenario '%s': cost %.2f, equity %.0f, duration %.2fs".
No new imports or helper functions are required; only that single logging call needs to be updated.
| @@ -129,9 +129,8 @@ | ||
| } | ||
| ) | ||
| _logger.info( | ||
| "Simulation completed for scenario '%s': %d beneficiaries, cost %.2f, equity %.0f, duration %.2fs", | ||
| "Simulation completed for scenario '%s': cost %.2f, equity %.0f, duration %.2fs", | ||
| scenario.name, | ||
| beneficiary_count, | ||
| total_cost, | ||
| equity_score, | ||
| duration, |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive new module, spp_simulation, for modeling and analyzing program targeting scenarios. While the overall structure is well-organized, a security audit identified several Cross-Site Scripting (XSS) vulnerabilities within the spp_simulation module, specifically in computed HTML fields and chatter messages. These issues arise from concatenating unescaped user-controlled strings, potentially allowing an attacker with 'Simulation Officer' privileges to execute arbitrary JavaScript. Additionally, the review highlights areas for improving maintainability by reducing code duplication, removing hardcoded values, and fixing inconsistencies between backend and frontend components, including refactoring duplicated logic in the comparison model, dynamically fetching selection labels, and addressing a bug in the simulation report.
| f"<tr><td>{mode}</td><td>{amount:,.2f}</td><td>{mult_field}</td>" | ||
| f"<td>{max_mult}</td><td><code>{cel_expr}</code></td>" | ||
| f"<td><code>{condition}</code></td></tr>" |
There was a problem hiding this comment.
Multiplier fields, CEL expressions, and condition expressions are included in the scenario snapshot HTML without escaping, posing an XSS risk.
| try: | ||
| dt = datetime.fromisoformat(executed_at.replace("Z", "+00:00")) | ||
| date_str = dt.strftime("%b %d, %Y %H:%M") | ||
| html_parts.append(f"<th>{name}<br/><small class='text-muted'>{date_str}</small></th>") |
There was a problem hiding this comment.
The scenario name is inserted into the HTML table header without escaping, leading to a potential Cross-Site Scripting (XSS) vulnerability. An attacker could craft a scenario name containing malicious scripts that would execute when another user views the comparison.
| rules_html += f"<li>Multiplier: {amount:,.2f} × {field}</li>" | ||
| elif mode == "cel": | ||
| cel = rule.get("amount_cel_expression", "?") | ||
| rules_html += f"<li>CEL: <code>{cel}</code></li>" |
There was a problem hiding this comment.
The CEL expression in entitlement rules is inserted into the HTML without escaping, posing an XSS risk.
| target_type_label = "households" if record.scenario_id.target_type == "group" else "individuals" | ||
| coverage_pct = f"{record.coverage_rate:.1f}" if record.coverage_rate else "0.0" | ||
| parts = [ | ||
| f"<strong>{scenario_name}</strong> targets " |
There was a problem hiding this comment.
The scenario name is included in the executive summary HTML without escaping. This is a stored XSS vulnerability.
| amount = area_info.get("amount", 0) | ||
| coverage = area_info.get("coverage_rate", 0) | ||
| html_parts.append( | ||
| f"<tr><td>{name}</td><td>{count:,}</td>" f"<td>{amount:,.2f}</td><td>{coverage:.1f}%</td></tr>" |
There was a problem hiding this comment.
Area names are included in the geographic breakdown HTML table without escaping, which can lead to XSS.
| # Post chatter message | ||
| scenario.message_post( | ||
| body=_("Converted to program: %s") % program.name, | ||
| ) |
There was a problem hiding this comment.
The program name is formatted into the chatter message body without escaping. Since Odoo chatter messages are rendered as HTML, this allows for a stored XSS attack via the program name.
| def action_compute_comparison(self): | ||
| """Compute the side-by-side comparison data.""" | ||
| self.ensure_one() | ||
| comparison_data = {"runs": []} | ||
| for run in self.run_ids: | ||
| run_data = { | ||
| "run_id": run.id, | ||
| "scenario_name": run.scenario_id.name, | ||
| "beneficiary_count": run.beneficiary_count, | ||
| "total_cost": run.total_cost, | ||
| "coverage_rate": run.coverage_rate, | ||
| "equity_score": run.equity_score, | ||
| "gini_coefficient": run.gini_coefficient, | ||
| "has_disparity": run.has_disparity, | ||
| "leakage_rate": run.leakage_rate, | ||
| "undercoverage_rate": run.undercoverage_rate, | ||
| "budget_utilization": run.budget_utilization, | ||
| "executed_at": run.executed_at.isoformat() if run.executed_at else None, | ||
| } | ||
| comparison_data["runs"].append(run_data) | ||
| self.comparison_json = comparison_data | ||
|
|
||
| # Build parameters comparison from snapshots | ||
| parameters_data = {"runs": []} | ||
| for run in self.run_ids: | ||
| snapshot = run.scenario_snapshot_json or {} | ||
| param_data = { | ||
| "run_id": run.id, | ||
| "scenario_name": run.scenario_id.name, | ||
| "executed_at": run.executed_at.isoformat() if run.executed_at else None, | ||
| "target_type": snapshot.get("target_type"), | ||
| "targeting_expression": snapshot.get("targeting_expression"), | ||
| "budget_amount": snapshot.get("budget_amount"), | ||
| "budget_strategy": snapshot.get("budget_strategy"), | ||
| "entitlement_rules": snapshot.get("entitlement_rules") or [], | ||
| "ideal_population_expression": snapshot.get("ideal_population_expression"), | ||
| } | ||
| parameters_data["runs"].append(param_data) | ||
| self.parameters_comparison_json = parameters_data | ||
|
|
||
| # Compute overlap if possible | ||
| self._compute_overlap() | ||
|
|
There was a problem hiding this comment.
There is significant code duplication between action_compute_comparison and _onchange_run_ids. Both methods build comparison_data and parameters_data using identical logic. To adhere to the DRY (Don't Repeat Yourself) principle and improve maintainability, this logic should be extracted into private helper methods.
For example, you could create _build_comparison_data(self) and _build_parameters_data(self) methods and call them from both places.
| return {} | ||
|
|
||
| partner_model = self.env["res.partner"] | ||
| amount_map = dict(zip(beneficiary_ids, amounts, strict=False)) |
There was a problem hiding this comment.
The zip function is used here. In Python 3.10+ (which Odoo 17 supports), zip has a strict argument. Since beneficiary_ids and amounts are expected to have the same length, using strict=True is safer as it will raise a ValueError if their lengths differ. This can help catch potential bugs early.
| amount_map = dict(zip(beneficiary_ids, amounts, strict=False)) | |
| amount_map = dict(zip(beneficiary_ids, amounts, strict=True)) |
| {key: "gini_coefficient", label: "Benefit Equality (Gini)", format: "decimal"}, | ||
| {key: "leakage_rate", label: "Leakage", format: "percent"}, | ||
| {key: "undercoverage_rate", label: "Missed Population", format: "percent"}, | ||
| {key: "targeting_accuracy", label: "Targeting Accuracy", format: "percent"}, |
There was a problem hiding this comment.
The metrics array includes 'targeting_accuracy', but the backend code in spp.simulation.comparison does not add this key to the comparison_json data. As a result, this metric will always appear as empty ('-') in the comparison table. You should either add the logic to compute and include this metric in the backend or remove it from the frontend component.
| const values = this.state.runs.map((r) => r[metricKey] || 0); | ||
| const value = values[runIndex]; | ||
| // For these metrics, lower is better | ||
| const lowerIsBetter = ["gini_coefficient", "leakage_rate", "undercoverage_rate", "total_cost"]; |
There was a problem hiding this comment.
The list of metrics where a lower value is better is hardcoded. This is fragile and requires updating this component if new 'lower-is-better' metrics are added. A more maintainable approach would be to include this information in the metric definition within the component's state.
For example:
{
key: 'gini_coefficient',
label: 'Benefit Equality (Gini)',
format: 'decimal',
lowerIsBetter: true
}Then isBestValue can check this property dynamically.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## 19.0 #46 +/- ##
===========================================
- Coverage 71.31% 42.44% -28.88%
===========================================
Files 299 141 -158
Lines 23618 10866 -12752
===========================================
- Hits 16844 4612 -12232
+ Misses 6774 6254 -520
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| "ideal_population_expression": snapshot.get("ideal_population_expression"), | ||
| } | ||
| parameters_data["runs"].append(param_data) | ||
| self.parameters_comparison_json = parameters_data |
There was a problem hiding this comment.
Duplicated comparison-building logic in onchange and action
Low Severity
_onchange_run_ids and action_compute_comparison contain nearly identical code blocks for building comparison_json and parameters_comparison_json. If the data structure changes, both copies need to be updated in sync, risking divergence and inconsistent behavior.
Additional Locations (1)
| "count": count, | ||
| "amount": area_amount, | ||
| "coverage_rate": count / total_count * 100 if total_count else 0, | ||
| } |
There was a problem hiding this comment.
Geographic "coverage_rate" actually computes distribution share
Medium Severity
The coverage_rate in the geographic breakdown is computed as count / total_count * 100 where total_count is the total number of beneficiaries — making it a distribution share (% of beneficiaries in this area), not a coverage rate (% of the area's population covered). The HTML column header labels this "Coverage," which is misleading in a social protection context where "coverage" has a specific per-area population meaning.
Additional Locations (1)
| total_registry_count = self._get_total_registry_count(scenario) | ||
|
|
||
| beneficiary_count = len(beneficiary_ids) | ||
| coverage_rate = (beneficiary_count / total_registry_count * 100) if total_registry_count else 0.0 |
There was a problem hiding this comment.
Beneficiary count not adjusted after cap_total budget
High Severity
beneficiary_count and coverage_rate are computed at line 58–59 before budget adjustment at line 65. When cap_total budget strategy zeroes out some beneficiaries' amounts, beneficiary_count still includes everyone who matched targeting—including those who receive nothing. The field's help text says "Number of registrants who would receive benefits," but this count is inflated. Additionally, the distribution statistics (Gini, percentiles) are computed on the post-cap amounts including the zeros, artificially inflating inequality measures.
Additional Locations (1)
| rule.multiplier_field or "unknown", | ||
| rule.amount, | ||
| ) | ||
| ) |
There was a problem hiding this comment.
Fixed entitlement rules missing explicit amount_mode in conversion
Medium Severity
In _create_wizard_entitlement_items, amount_mode is explicitly set for "cel" and "multiplier" rules but not for "fixed" rules — the most common case. When rule.amount_mode == "fixed", the item_vals dict is created without an amount_mode key, relying entirely on the wizard item's field default. If that default ever changes or differs from "fixed", all fixed-amount rules would be created with the wrong mode during scenario-to-program conversion.
| return value === Math.min(...values); | ||
| } | ||
| return value === Math.max(...values); | ||
| } |
There was a problem hiding this comment.
Comparison table highlights uncomputed metrics as best
Medium Severity
isBestValue uses r[metricKey] || 0 which converts null/undefined (meaning "not computed") to 0. For lower-is-better metrics like leakage_rate and undercoverage_rate, an uncomputed metric becomes 0 and gets highlighted green as "best value." Meanwhile, formatValue correctly displays null as "-", so a cell can show "-" text with misleading green "best" highlighting. This happens whenever one scenario has an ideal population expression and the other doesn't.
| base_domain=[("disabled", "=", False)], | ||
| limit=0, | ||
| ) | ||
| record.targeting_preview_count = result.get("count", 0) |
There was a problem hiding this comment.
Preview count uses different targeting path than simulation
Medium Severity
_compute_targeting_preview_count uses cel_service.compile_expression with a hardcoded base_domain=[("disabled", "=", False)], while the actual simulation in _execute_targeting uses spp.cel.executor.compile_for_batch with the profile's base domain loaded from spp.cel.registry.load_profile(). These are entirely different code paths with different base domains. The profile's base domain likely includes additional filters (e.g., is_registrant, is_group), so the preview count displayed in the scenario form can differ significantly from the actual simulation result, misleading users who rely on it to estimate targeting reach.
Additional Locations (1)
…ling New module providing: - Scenario templates and configurable simulation scenarios - Simulation run execution with metric tracking - Scenario-to-program conversion for promoting simulations to real programs - Comparison wizard for side-by-side scenario analysis - Targeting efficiency and coverage metrics
…ng targeting_accuracy field - Add markupsafe Markup/escape imports and use Markup.format() for all HTML computed fields in simulation_run.py and simulation_comparison.py to safely escape user-controlled values (scenario names, area names, metric names, CEL expressions, targeting expressions, multiplier fields) - Use Markup() for static HTML strings in html_parts lists so join works correctly - Escape program.name in simulation_service.py chatter message_post body - Replace non-existent doc.targeting_accuracy field in simulation_report.xml with a condition on the existing leakage_rate/undercoverage_rate fields - Remove targeting_accuracy from comparison_table.js metrics array
…allback logic - Load CEL profile in _get_ideal_population_ids so the executor runs with the correct base domain and model context (matches _execute_targeting) - Fix _compute_metric_results_html to read "rate" and "ratio" keys for coverage and ratio metrics instead of always reading "value" - Fix distribution stats to use explicit None check so zero values render as numbers instead of dashes - Add "scenario_id.target_type" to @api.depends for _compute_summary_html - Set amount_mode = "fixed" in the multiplier-to-fixed fallback branch so the wizard item is created with an explicit mode - Fix aggregate metric to use metric.aggregation as the result key so that sum/avg/min/max aggregations read the correct value from the CEL result
jeremi
force-pushed
the
feat/simulation-engine
branch
from
3393dca to
b03812f
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
| "res.partner", scenario.targeting_expression, batch_size=5000 | ||
| ): | ||
| all_ids.extend(batch_ids) | ||
| run_sets[run.id] = set(all_ids) |
There was a problem hiding this comment.
Overlap computation uses current expression instead of snapshot
Medium Severity
_compute_overlap reads run.scenario_id.targeting_expression (the current scenario state) instead of using the targeting expression from run.scenario_snapshot_json that was captured at simulation execution time. If a scenario's targeting expression is modified after a run completes, the overlap computation will use the new expression, producing results that don't match the actual simulation run data. The snapshot data is readily available on each run record but is not used here.
1 participant
Summary
Dependencies
spp_aggregationandspp_metrics_coreOrigin
From
openspp-modules-v2branchclaude/global-alliance-policy-basket.Test plan
Note
Medium Risk
Large new module touching eligibility/expression execution and program creation flows; while it stores only aggregates, simulation/overlap recomputation against live registry data and conversion automation could have functional/performance implications.
Overview
Introduces a new Odoo addon
spp_simulationthat lets users define CEL-based targeting scenarios (with templates and live preview counts), run non-deletable simulations, and persist only aggregated results for coverage, cost, distribution (incl. Gini/percentiles), fairness/parity, geographic breakdown, targeting efficiency (leakage/undercoverage), and custom metrics.Adds scenario-to-program conversion via the existing program creation wizard, plus run comparison features including recomputable side-by-side metrics, parameter snapshot tables, and current-registry overlap/Jaccard analysis, with new OWL widgets and a PDF report for viewing results. Includes new menus, security groups/access rules, seeded scenario templates, and a comprehensive test suite enforcing constraints, privacy guarantees, and access controls.
Written by Cursor Bugbot for commit b03812f. This will update automatically on new commits. Configure here.