Complete portfolio mutation audit coverage#121
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7b415ab3a6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| _audit_failure( | ||
| actor, | ||
| PortfolioAuditSource.API, | ||
| "order_upsert", | ||
| PortfolioAuditEntityType.ACTIVE_ORDER, |
There was a problem hiding this comment.
This failure audit only runs from the except ValueError block, but _to_decimal(...) raises HTTPException for malformed decimal fields. For an authenticated order request with a known ticker and shares, limit_price, or stop_price set to a non-decimal value, the route returns 422 before _audit_failure runs, so the failed mutation is omitted from the audit log even though the portfolio and entity are identifiable.
Useful? React with 👍 / 👎.
| async def admin_trade_buy(request: Request, actor: str = Depends(admin_auth)): | ||
| try: | ||
| return await _trade(request, "buy") | ||
| return await _trade(request, "buy", actor) |
There was a problem hiding this comment.
When _trade raises after parsing the form, such as an admin sell attempt for more shares than are held or an invalid trade amount/date, control jumps to this handler and returns the 422 page without recording a failed trade_buy/trade_sell audit event. That leaves identifiable failed admin trade mutations out of the audit trail while the neighboring cash/order/timeline/risk failure paths are logged.
Useful? React with 👍 / 👎.
### Motivation - Provide consistent audit logging for all API and admin portfolio mutation paths using the existing audit helpers and enums. - Ensure failed-validation attempts record safe, non-sensitive audit events where the portfolio can be identified. - Capture compact before/after snapshots for affected entities to improve audit usefulness without leaking secrets. ### Description - Added small local helpers in `src/finwall/api.py` (`_one`, `_goal_snapshot`, `_timeline_snapshot`, `_risk_snapshot`, `_trade_snapshot`, `_audit_failure`) to produce compact snapshots and to record sanitized failed-audit events while reusing `record_update_audit` and existing enums. - Instrumented API mutation routes to record successful audit events and safe failed-validation events where practical for `cash_withdraw`, `holding_delete`, `trade_buy`, `trade_sell`, `order_upsert`, `order_delete`, `watchlist_upsert`, `watchlist_delete`, `goal_set`, `timeline_set`, and `risk_profile_set` using `PortfolioAuditSource.API` and the authenticated actor. - Instrumented admin form handlers to record the same successful (and representative failed) audit events using `PortfolioAuditSource.WEB` and the admin actor, and added before/after snapshots where practical without changing any business logic. - Kept summaries compact and used `safe_error_message` to populate `safe_error_message` for failed events; avoided storing tokens, cookies, headers, or raw tracebacks. - Updated `tests/test_api.py` with focused tests that assert API/admin mutation audit coverage and that failed-validation audit events are present and sanitized (`test_api_mutation_audit_coverage`, `test_failed_validation_audit_events_are_safe`, `test_admin_mutation_audit_coverage`). ### Testing - Ran lint/format checks with `poetry run ruff check .` and `poetry run ruff format --check --line-length 88 .`, both passed. - Ran the full test suite with `poetry run pytest`, and all tests passed (282 passed). - Added and executed the new API tests in `tests/test_api.py`, which assert presence of audit events, actor/source correctness, snapshots, and sanitized failed-audit messages; these tests passed as part of the suite.
1 participant
Motivation
Description
src/finwall/api.py(_one,_goal_snapshot,_timeline_snapshot,_risk_snapshot,_trade_snapshot,_audit_failure) to produce compact snapshots and to record sanitized failed-audit events while reusingrecord_update_auditand existing enums.cash_withdraw,holding_delete,trade_buy,trade_sell,order_upsert,order_delete,watchlist_upsert,watchlist_delete,goal_set,timeline_set, andrisk_profile_setusingPortfolioAuditSource.APIand the authenticated actor.PortfolioAuditSource.WEBand the admin actor, and added before/after snapshots where practical without changing any business logic.safe_error_messageto populatesafe_error_messagefor failed events; avoided storing tokens, cookies, headers, or raw tracebacks.tests/test_api.pywith focused tests that assert API/admin mutation audit coverage and that failed-validation audit events are present and sanitized (test_api_mutation_audit_coverage,test_failed_validation_audit_events_are_safe,test_admin_mutation_audit_coverage).Testing
poetry run ruff check .andpoetry run ruff format --check --line-length 88 ., both passed.poetry run pytest, and all tests passed (282 passed).tests/test_api.py, which assert presence of audit events, actor/source correctness, snapshots, and sanitized failed-audit messages; these tests passed as part of the suite.Codex Task