Merged
Conversation
Contributor
Reviewer's GuideRefactors the CLI surf-report pipeline for better testability and logging, converts helper flags to booleans, centralizes Open-Meteo client creation, improves error handling and database/email/server behavior, and adds broad unit-test coverage and coverage configuration. Sequence diagram for SurfReport.run end-to-end surf report generationsequenceDiagram
actor User
participant CLI as cli.run
participant SR as SurfReport
participant Helper as helper
participant API as api
participant DB as SurfReportDatabaseOps
participant GPT as gpt
User->>CLI: invoke run(lat, long, args)
CLI->>SR: create SurfReport()
CLI->>SR: run(lat, long, args)
SR->>Helper: separate_args(args or sys.argv)
Helper-->>SR: parsed_args
SR->>API: separate_args_and_get_location(parsed_args)
API-->>SR: {lat, long, city}
SR->>Helper: set_location(location_data)
Helper-->>SR: city, loc_lat, loc_long
SR->>SR: choose lat, long (explicit or from location)
SR->>Helper: arguments_dictionary(lat, long, city, parsed_args)
Helper-->>SR: arguments
SR->>API: gather_data(lat, long, arguments)
API->>API: _create_openmeteo_client()
API->>ExternalOpenMeteo: API calls (uv, ocean, forecast, hourly, rain)
ExternalOpenMeteo-->>API: responses
API-->>SR: ocean_data_dict
alt database_enabled
SR->>DB: insert_report(ocean_data_dict)
DB-->>SR: inserted_id
end
alt json_output is False
SR->>Helper: print_outputs(ocean_data_dict, arguments, gpt_prompt, gpt_info)
Helper->>GPT: simple_gpt or openai_gpt
GPT-->>Helper: gpt_response
Helper-->>SR: gpt_response
SR-->>CLI: (ocean_data_dict, gpt_response)
else json_output is True
SR->>Helper: json_output(ocean_data_dict)
Helper-->>SR: ocean_data_dict
SR-->>CLI: ocean_data_dict
end
Sequence diagram for send_user_email surf report email flowsequenceDiagram
actor Scheduler
participant EmailMod as send_email
participant Env as EmailSettings
participant Curl as curl_command
participant SMTP as SMTP_server
Scheduler->>EmailMod: send_user_email()
EmailMod->>Env: load EmailSettings
Env-->>EmailMod: EMAIL, EMAIL_RECEIVER, SUBJECT, COMMAND, SMTP settings
EmailMod->>Curl: subprocess.run(["curl", COMMAND])
alt curl_success
Curl-->>EmailMod: stdout surf_report
EmailMod->>EmailMod: body = stdout
else curl_failure
Curl-->>EmailMod: CalledProcessError
EmailMod->>EmailMod: log error, body = failure message
end
EmailMod->>SMTP: connect, starttls, login
EmailMod->>SMTP: sendmail(EMAIL, EMAIL_RECEIVER, message)
SMTP-->>EmailMod: ok
EmailMod-->>Scheduler: return
Class diagram for SurfReport orchestration and database integrationclassDiagram
class SurfReport {
+SurfReport()
+run(lat, long, args)
+_init_db()
+_save_report(ocean_data_dict)
+_render_output(ocean_data_dict, arguments)
-gpt_prompt
-gpt_info
-db_handler
}
class GPTSettings {
+GPT_PROMPT
+API_KEY
+GPT_MODEL
}
class DatabaseSettings {
+DB_URI
}
class SurfReportDatabaseOps {
+SurfReportDatabaseOps()
+insert_report(report_document)
-db
-collection
}
class Database {
+Database()
+connect(db_name)
+disconnect()
-db_uri
-client
-db
}
class db_manager {
}
SurfReport --> GPTSettings : loads
SurfReport --> SurfReportDatabaseOps : optionally_creates
SurfReport --> DatabaseSettings : checks_DB_URI
SurfReportDatabaseOps --> db_manager : uses
db_manager --> Database : instance_of
DatabaseSettings <|-- GPTSettings
class EmailSettings {
+EMAIL
+EMAIL_PW
+EMAIL_RECEIVER
+COMMAND
+SUBJECT
}
EmailSettings --> ServerSettings
class ServerSettings {
+HOST
+PORT
+DEBUG
+IP_ADDRESS
}
class CommonSettings {
+model_dump()
}
CommonSettings <|-- ServerSettings
CommonSettings <|-- GPTSettings
CommonSettings <|-- DatabaseSettings
CommonSettings <|-- EmailSettings
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| json_out = json.dumps(data_dict, indent=4) | ||
| if print_output: | ||
| print(json_out) | ||
| print(json.dumps(data_dict, indent=4)) |
Check failure
Code scanning / CodeQL
Clear-text logging of sensitive information High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 19 days ago
In general, to fix this kind of issue you either (a) avoid emitting sensitive fields at all, (b) mask or generalize them before emitting, or (c) make the emission an explicit, opt‑in choice separated from routine logging. Here we want to keep JSON output useful but not unconditionally dump precise coordinates and city information, which CodeQL has flagged as sensitive.
The best minimal change without altering external behavior for current callers is to have json_output support optional redaction of location-identifying fields and then use that from the CLI JSON path. Concretely:
- Extend
json_output’s signature insrc/helper.pyto accept a new boolean parameter, e.g.redact_location=False. - When
redact_locationisTrue, create a shallow copy ofdata_dictand remove or mask the fields that can be derived from user location:"Lat","Long", and"Location"(keys used inocean_data_dict). - Serialize and print the possibly-redacted copy instead of the original
data_dict. Always return the originaldata_dictto avoid changing programmatic behavior. - In
SurfReport._render_outputinsrc/cli.py, when callinghelper.json_output, passredact_location=Trueso that the default CLI JSON output does not include precise coordinates or city name. - This keeps arguments, gathering, and printing behavior unchanged for all other paths, and does not introduce new dependencies (we only use
dict.copy()anddict.popfrom the standard library).
This single change addresses all alert variants pointing to that print of json.dumps(data_dict, ...), because any ocean_data_dict originating from user location will now have sensitive fields removed before being printed in JSON mode.
Suggested changeset
2
src/helper.py
| @@ -284,13 +284,23 @@ | ||
| return [round(num, decimal) for num in round_list] | ||
|
|
||
|
|
||
| def json_output(data_dict, print_output=True): | ||
| def json_output(data_dict, print_output=True, redact_location=False): | ||
| """ | ||
| Serializes data_dict to JSON. Prints to stdout if print_output is True. | ||
| Returns the original dict for programmatic use. | ||
|
|
||
| When redact_location is True, location-identifying fields such as | ||
| coordinates and city name are removed from the printed JSON to | ||
| avoid logging potentially sensitive data. | ||
| """ | ||
| output_dict = data_dict | ||
| if redact_location and isinstance(data_dict, dict): | ||
| output_dict = data_dict.copy() | ||
| for key in ("Lat", "Long", "Location"): | ||
| output_dict.pop(key, None) | ||
|
|
||
| if print_output: | ||
| print(json.dumps(data_dict, indent=4)) | ||
| print(json.dumps(output_dict, indent=4)) | ||
| return data_dict | ||
|
|
||
|
|
src/cli.py
Outside changed files
| @@ -72,7 +72,7 @@ | ||
| ocean_data_dict, arguments, self.gpt_prompt, self.gpt_info | ||
| ) | ||
| return ocean_data_dict, response | ||
| helper.json_output(ocean_data_dict) | ||
| helper.json_output(ocean_data_dict, redact_location=True) | ||
| return ocean_data_dict | ||
|
|
||
|
|
Copilot is powered by AI and may make mistakes. Always verify output.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 1 security issue, 4 other issues, and left some high level feedback:
Security issues:
- Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'. (link)
General comments:
- The change to
get_rain(lat, long)(dropping thedecimalparameter) alters the public function signature; if this is used outsidegather_data, consider keeping a deprecated wrapper or adding*args, **kwargsfor backward compatibility to avoid breaking external callers. - In
server.default_route, re-raisingCalledProcessErrornow leads to a raw 500 response; you may want to catch the exception and return a user-friendly error payload (and appropriate status) instead of propagating the traceback to the client. - Now that
DEFAULT_ARGUMENTSuses booleans, it might be helpful to assert or validate argument types at the boundary (e.g., inarguments_dictionaryorset_output_values) to avoid subtle bugs if other code still passes0/1or non-bool values.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The change to `get_rain(lat, long)` (dropping the `decimal` parameter) alters the public function signature; if this is used outside `gather_data`, consider keeping a deprecated wrapper or adding `*args, **kwargs` for backward compatibility to avoid breaking external callers.
- In `server.default_route`, re-raising `CalledProcessError` now leads to a raw 500 response; you may want to catch the exception and return a user-friendly error payload (and appropriate status) instead of propagating the traceback to the client.
- Now that `DEFAULT_ARGUMENTS` uses booleans, it might be helpful to assert or validate argument types at the boundary (e.g., in `arguments_dictionary` or `set_output_values`) to avoid subtle bugs if other code still passes `0/1` or non-bool values.
## Individual Comments
### Comment 1
<location path="pyproject.toml" line_range="59-61" />
<code_context>
+
+[tool.coverage.report]
+omit = ["src/dev_streamlit.py"]
+exclude_lines = [
+ "pragma: no cover",
+ "if __name__ == .__main__.:",
+]
+
</code_context>
<issue_to_address>
**nitpick (testing):** The coverage exclude_lines pattern for __main__ guards looks incorrect.
As written, this string won’t match `if __name__ == "__main__":`, so those blocks won’t be excluded from coverage. Consider using the exact string literal (`"if __name__ == \"__main__\":"`) or a correctly escaped regex pattern, depending on how coverage interprets `exclude_lines`.
</issue_to_address>
### Comment 2
<location path="tests/test_helper.py" line_range="238-244" />
<code_context>
+ assert "Forecast days must be between" in caplog.text
+
+
+def test_get_forecast_days_negative_logs_warning(caplog):
+ """get_forecast_days returns 0 and warns when value is negative."""
+ with caplog.at_level(logging.WARNING, logger="src.helper"):
+ result = helper.get_forecast_days(["forecast=-1"])
+ assert result == 0
+
+
</code_context>
<issue_to_address>
**suggestion (testing):** Also assert the warning message for negative forecast days to fully verify the logging behavior.
In `test_get_forecast_days_negative_logs_warning`, mirror the pattern from `test_get_forecast_days_out_of_range_logs_warning` by asserting that a warning substring like `"Forecast days must be between"` appears in `caplog.text`. This keeps the tests consistent and better guards against regressions in the logged message.
```suggestion
def test_get_forecast_days_negative_logs_warning(caplog):
"""get_forecast_days returns 0 and warns when value is negative."""
with caplog.at_level(logging.WARNING, logger="src.helper"):
result = helper.get_forecast_days(["forecast=-1"])
assert result == 0
assert "Forecast days must be between" in caplog.text
```
</issue_to_address>
### Comment 3
<location path="tests/test_server.py" line_range="17-23" />
<code_context>
-# response_root = test_client.get("/")
-# assert response_root.status_code == OK
+def test_serve_index_returns_200(monkeypatch):
+ """GET /home renders the index template and returns 200."""
+ app = _make_app()
+ with patch("src.server.render_template", return_value="<html>home</html>"):
+ resp = app.test_client().get("/home")
+ assert resp.status_code == HTTPStatus.OK
+ assert b"<html>home</html>" in resp.data
+
+
</code_context>
<issue_to_address>
**suggestion (testing):** Add a happy-path test for the root route (`/`) when the subprocess succeeds.
You already cover `/home`, `/script.js`, and the error case for `/` when `CalledProcessError` is raised. Please also add a test where `subprocess.run` succeeds and returns a known `stdout`, and assert that `GET /` responds with 200 and includes that content. This will ensure the main success path for `/` is tested as well.
Suggested implementation:
```python
from src.settings import ServerSettings
def test_serve_root_returns_200_on_success(monkeypatch):
"""GET / executes the script successfully and returns its stdout with 200."""
app = _make_app()
completed = subprocess.CompletedProcess(
args=["dummy-script"],
returncode=0,
stdout="hello from script",
)
# Patch the subprocess call used by the root route to simulate a successful run
with patch("src.server.subprocess.run", return_value=completed):
resp = app.test_client().get("/")
assert resp.status_code == HTTPStatus.OK
assert b"hello from script" in resp.data
```
This assumes:
1. The root route (`/`) uses `subprocess.run` via `import subprocess` inside `src.server`.
*If `src.server` instead does `from subprocess import run`, change the patch target to `"src.server.run"` accordingly.*
2. The root handler includes `stdout` directly (or via a template) in the response body; if it wraps or prefixes the text, adjust the asserted substring (`b"hello from script"`) to match the actual rendered content.
</issue_to_address>
### Comment 4
<location path="tests/test_send_email.py" line_range="50-59" />
<code_context>
+ mock_smtp.sendmail.assert_called_once()
+
+
+def test_send_email_curl_failure_uses_fallback_body(mocker):
+ """send_user_email falls back to an error message when curl fails."""
+ _patch_env(mocker)
+
+ mocker.patch(
+ "subprocess.run",
+ side_effect=subprocess.CalledProcessError(1, "curl", stderr="error"),
+ )
+
+ mock_smtp = MagicMock()
+ mocker.patch("smtplib.SMTP", return_value=mock_smtp)
+ mock_smtp.__enter__ = lambda s: s
+ mock_smtp.__exit__ = MagicMock(return_value=False)
+
+ # Should not raise; the fallback body is used instead
+ send_user_email()
+
+ mock_smtp.sendmail.assert_called_once()
+ # The email body should contain the fallback message
+ call_args = mock_smtp.sendmail.call_args[0]
+ assert "Failed to fetch surf report." in call_args[2]
</code_context>
<issue_to_address>
**suggestion (testing):** Consider adding a test for SMTP failures to cover that error path as well.
You already cover the happy path and curl failure fallback. To fully exercise `send_user_email`’s error handling, add a test where the `SMTP` mock raises (e.g., on `server.login` or `sendmail`). This will verify the behavior when SMTP itself fails (whether you choose to propagate or log the error).
</issue_to_address>
### Comment 5
<location path="src/server.py" line_range="62-67" />
<code_context>
result = subprocess.run(
[sys.executable, Path("src") / "cli.py", args],
capture_output=True,
text=True,
check=True,
)
</code_context>
<issue_to_address>
**security (python.lang.security.audit.dangerous-subprocess-use-audit):** Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'.
*Source: opengrep*
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+59
to
+61
| exclude_lines = [ | ||
| "pragma: no cover", | ||
| "if __name__ == .__main__.:", |
Contributor
There was a problem hiding this comment.
nitpick (testing): The coverage exclude_lines pattern for main guards looks incorrect.
As written, this string won’t match if __name__ == "__main__":, so those blocks won’t be excluded from coverage. Consider using the exact string literal ("if __name__ == \"__main__\":") or a correctly escaped regex pattern, depending on how coverage interprets exclude_lines.
Comment on lines
+238
to
+244
| def test_get_forecast_days_negative_logs_warning(caplog): | ||
| """get_forecast_days returns 0 and warns when value is negative.""" | ||
| with caplog.at_level(logging.WARNING, logger="src.helper"): | ||
| result = helper.get_forecast_days(["forecast=-1"]) | ||
| assert result == 0 | ||
|
|
||
|
|
Contributor
There was a problem hiding this comment.
suggestion (testing): Also assert the warning message for negative forecast days to fully verify the logging behavior.
In test_get_forecast_days_negative_logs_warning, mirror the pattern from test_get_forecast_days_out_of_range_logs_warning by asserting that a warning substring like "Forecast days must be between" appears in caplog.text. This keeps the tests consistent and better guards against regressions in the logged message.
Suggested change
| def test_get_forecast_days_negative_logs_warning(caplog): | |
| """get_forecast_days returns 0 and warns when value is negative.""" | |
| with caplog.at_level(logging.WARNING, logger="src.helper"): | |
| result = helper.get_forecast_days(["forecast=-1"]) | |
| assert result == 0 | |
| def test_get_forecast_days_negative_logs_warning(caplog): | |
| """get_forecast_days returns 0 and warns when value is negative.""" | |
| with caplog.at_level(logging.WARNING, logger="src.helper"): | |
| result = helper.get_forecast_days(["forecast=-1"]) | |
| assert result == 0 | |
| assert "Forecast days must be between" in caplog.text | |
Comment on lines
+17
to
+23
| def test_serve_index_returns_200(monkeypatch): | ||
| """GET /home renders the index template and returns 200.""" | ||
| app = _make_app() | ||
| with patch("src.server.render_template", return_value="<html>home</html>"): | ||
| resp = app.test_client().get("/home") | ||
| assert resp.status_code == HTTPStatus.OK | ||
| assert b"<html>home</html>" in resp.data |
Contributor
There was a problem hiding this comment.
suggestion (testing): Add a happy-path test for the root route (/) when the subprocess succeeds.
You already cover /home, /script.js, and the error case for / when CalledProcessError is raised. Please also add a test where subprocess.run succeeds and returns a known stdout, and assert that GET / responds with 200 and includes that content. This will ensure the main success path for / is tested as well.
Suggested implementation:
from src.settings import ServerSettings
def test_serve_root_returns_200_on_success(monkeypatch):
"""GET / executes the script successfully and returns its stdout with 200."""
app = _make_app()
completed = subprocess.CompletedProcess(
args=["dummy-script"],
returncode=0,
stdout="hello from script",
)
# Patch the subprocess call used by the root route to simulate a successful run
with patch("src.server.subprocess.run", return_value=completed):
resp = app.test_client().get("/")
assert resp.status_code == HTTPStatus.OK
assert b"hello from script" in resp.dataThis assumes:
- The root route (
/) usessubprocess.runviaimport subprocessinsidesrc.server.
Ifsrc.serverinstead doesfrom subprocess import run, change the patch target to"src.server.run"accordingly. - The root handler includes
stdoutdirectly (or via a template) in the response body; if it wraps or prefixes the text, adjust the asserted substring (b"hello from script") to match the actual rendered content.
Comment on lines
+50
to
+59
| def test_send_email_curl_failure_uses_fallback_body(mocker): | ||
| """send_user_email falls back to an error message when curl fails.""" | ||
| _patch_env(mocker) | ||
|
|
||
| mocker.patch( | ||
| "subprocess.run", | ||
| side_effect=subprocess.CalledProcessError(1, "curl", stderr="error"), | ||
| ) | ||
|
|
||
| mock_smtp = MagicMock() |
Contributor
There was a problem hiding this comment.
suggestion (testing): Consider adding a test for SMTP failures to cover that error path as well.
You already cover the happy path and curl failure fallback. To fully exercise send_user_email’s error handling, add a test where the SMTP mock raises (e.g., on server.login or sendmail). This will verify the behavior when SMTP itself fails (whether you choose to propagate or log the error).
Comment on lines
+62
to
+67
| result = subprocess.run( | ||
| [sys.executable, Path("src") / "cli.py", args], | ||
| capture_output=True, | ||
| text=True, | ||
| check=True, | ||
| ) |
Contributor
There was a problem hiding this comment.
security (python.lang.security.audit.dangerous-subprocess-use-audit): Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'.
Source: opengrep
Codecov Report✅ All modified and coverable lines are covered by tests.
... and 1 file with indirect coverage changes 🚀 New features to boost your workflow:
|
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.
General:
Code:
Summary by Sourcery
Refactor the CLI surf report pipeline into a clearer, testable architecture with stronger logging, configuration defaults, and reliability improvements across API, GPT, server, email, and database components.
New Features:
Bug Fixes:
Enhancements:
Build:
Documentation:
Tests: