Repository navigation
AOJ-459: Add event trigger path - #76
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a TypeScript polling pipeline that detects SimpleFIN deposit transactions above a threshold, deduplicates via in-memory and file-backed stores, spawns a Python buy-ticket subprocess per detection with a sanitized BUY_TICKET_* env, logs runs, and propagates trigger context into Python ticket payloads and logs. ChangesDeposit Trigger Pipeline
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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: fbf1e52d30
ℹ️ 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".
|
|
||
| const projectRoot = path.resolve(import.meta.dir, "..", "..", ".."); | ||
| const client = createClient(accessUrl); | ||
| const seenStore = createMemorySeenStore(); |
There was a problem hiding this comment.
When bun run trigger is used as a scheduled one-shot command, or the --watch process restarts, this in-memory store starts empty while each poll fetches the last 7 days of transactions. Any deposit already processed in that lookback will therefore trigger another buy-ticket smoke run and notification because the prior log/state files are not consulted. Use a persisted seen store keyed by transactionKey so duplicate suppression survives process boundaries.
Useful? React with 👍 / 👎.
|
| Filename | Overview |
|---|---|
| apps/simplefin-sync/src/trigger.ts | New file implementing deposit detection, deduplication (memory + file stores), failure capping, credential scrubbing from child env, and audit log writing; all previously flagged issues are addressed. |
| apps/simplefin-sync/src/trigger.test.ts | New test file with comprehensive coverage: detection threshold, intra-poll deduplication, cross-store-instance persistence, failure capping, log-write-failure atomicity, and env sanitization. |
| buy_ticket_agent/pipeline.py | Adds _trigger_context_from_env() to read sanitized trigger metadata from env, and propagates it into both the ticket payload and the run log result dict. |
| tests/python/test_buy_ticket_agent_smoke.py | Adds a test verifying that trigger context env vars appear in both the draft ticket and the run log when run_smoke is called with trigger env set. |
| apps/simplefin-sync/package.json | Adds trigger, test, and typecheck scripts consistent with Bun tooling conventions. |
Sequence Diagram
sequenceDiagram
participant CLI as trigger.ts CLI
participant Poll as pollDepositTriggers
participant SFIN as SimpleFIN API
participant Store as SeenTransactionStore
participant Proc as buy_ticket_agent
participant Log as Audit Log
participant Py as pipeline.py
CLI->>Poll: call with client, seenStore, projectRoot
Poll->>SFIN: "fetchAccounts startDate, pending=true"
SFIN-->>Poll: SfinAccountSet
Poll->>Store: has(transactionKey) for each deposit over threshold
Store-->>Poll: seen or unseen
Poll->>Proc: Bun.spawn uv run python -m buy_ticket_agent.main --smoke
Note over Proc,Py: BUY_TICKET_TRIGGER_* env vars injected, SIMPLEFIN_* stripped
Proc->>Py: run_smoke()
Py->>Py: _trigger_context_from_env()
Py-->>Proc: SmokeResult with trigger context in draft and log
Proc-->>Poll: exitCode, stdoutChars, stderrChars
Poll->>Log: writeTriggerLog JSON
Poll->>Store: mark on success, markFailure on non-zero exit
Poll-->>CLI: PollResult triggered, failed, cappedFailures, skippedDuplicates
Reviews (3): Last reviewed commit: "fix(aoj-459): settle trigger review find..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
apps/simplefin-sync/src/trigger.test.ts (1)
1-3: ⚡ Quick winUse
Bun.filefor log reads in this TS test.Please replace
readFileusage withBun.file(...).text()to match the repo’s Bun-first file I/O guideline.Suggested patch
-import { mkdtemp, readdir, readFile, rm } from "node:fs/promises"; +import { mkdtemp, readdir, rm } from "node:fs/promises"; @@ - const log = JSON.parse(await readFile(path.join(logDir, logFile), "utf8")); + const log = JSON.parse(await Bun.file(path.join(logDir, logFile)).text());As per coding guidelines, "
**/*.{ts,tsx,js,jsx}: PreferBun.fileovernode:fs's readFile/writeFile".Also applies to: 138-138
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/simplefin-sync/src/trigger.test.ts` around lines 1 - 3, Tests import and use node:fs/promises.readFile; replace any readFile(...) calls in this test (and the occurrence around line 138) with Bun.file(pathToFile).text() to follow the Bun-first I/O guideline—locate uses of readFile in this file (e.g., references in trigger.test.ts) and change them to call Bun.file(<same path>).text(), removing the readFile import if no longer used and keeping mkdtemp/readdir/rm imports as needed.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/simplefin-sync/src/trigger.ts`:
- Around line 280-289: Wrap the call to pollDepositTriggers(...) inside the
do/while loop in a try/catch so transient exceptions don't kill the process
during watch mode: call pollDepositTriggers with client, seenStore and
projectRoot inside try; on error, if watch is true log the error (including
context) and await Bun.sleep(intervalMs) before continuing the loop; if watch is
false rethrow the error so non-watch behavior stays the same. Ensure the catch
still awaits Bun.sleep(intervalMs) to avoid a tight retry loop and reference
pollDepositTriggers, seenStore, client, projectRoot, watch, and intervalMs when
locating the change.
- Around line 171-186: The runBuyTicketSubprocess function currently spawns a
child process without a timeout; add an AbortController-based timeout and pass
its signal to Bun.spawn (or otherwise ensure the process is killed) so the
subprocess can't hang forever. Concretely: create an AbortController and set a
timer (e.g., configurable constant) that calls controller.abort() and
proc.kill() on timeout, pass controller.signal into Bun.spawn options, and
ensure the timer is cleared once proc.exited resolves; also handle the abort
path so you still await countStreamChars(proc.stdout/stderr) and proc.exited (or
return a failure TriggerResult) when the process is aborted. Reference
runBuyTicketSubprocess, buildBuyTicketCommand, buildBuyTicketEnv,
countStreamChars and proc.exited/proc.kill when making the change.
---
Nitpick comments:
In `@apps/simplefin-sync/src/trigger.test.ts`:
- Around line 1-3: Tests import and use node:fs/promises.readFile; replace any
readFile(...) calls in this test (and the occurrence around line 138) with
Bun.file(pathToFile).text() to follow the Bun-first I/O guideline—locate uses of
readFile in this file (e.g., references in trigger.test.ts) and change them to
call Bun.file(<same path>).text(), removing the readFile import if no longer
used and keeping mkdtemp/readdir/rm imports as needed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4c06ad8c-42d7-434d-a3c3-307c841fb918
📒 Files selected for processing (5)
apps/simplefin-sync/package.jsonapps/simplefin-sync/src/trigger.test.tsapps/simplefin-sync/src/trigger.tsbuy_ticket_agent/pipeline.pytests/python/test_buy_ticket_agent_smoke.py
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/simplefin-sync/src/trigger.ts`:
- Around line 35-39: The SeenTransactionStore interface allows optional
failureCount/markFailure which lets custom stores silently ignore maxFailures;
fix by adding a fallback internal failure tracker when failureCount or
markFailure are missing: in the code paths that check failures (where you
currently call store.failureCount(key) and store.markFailure(key)), detect if
those methods are undefined on the provided SeenTransactionStore and, if so, use
an internal Map/WeakMap to track per-key failure counts and increment/read from
that map so the configured maxFailures is honored; update the logic that decides
whether to requeue/retry to consult this fallback tracker exactly the same way
it would consult store.failureCount/markFailure so behavior is consistent.
- Around line 96-125: The persist() function currently writes directly to
statePath which can leave a truncated JSON on crash; change persist() to write
atomically by writing the JSON to a temp file in the same directory (e.g.,
statePath + ".tmp" or using a unique suffix), flush/fsync the temp file, then
rename/move the temp file to statePath (atomic rename) and remove the temp on
errors; keep the existing JSON payload built from seen and failures, ensure
mkdir(path.dirname(statePath), { recursive: true }) runs before creating the
temp, and handle/propagate I/O errors so load() can continue to assume a valid
JSON file.
- Around line 449-460: When handling errors from pollDepositTriggers, scrub the
error using pollErrorPayload before exiting in non-watch mode; replace the
current direct rethrow with first logging
console.error(JSON.stringify(pollErrorPayload(error))) and then rethrow the
original error (or throw a new Error if you prefer) so the CLI path uses the
scrubbed payload, while preserving the existing watch behavior that logs the
scrubbed payload, awaits Bun.sleep(intervalMs), and continues; update the catch
block around pollDepositTriggers to call pollErrorPayload(error) in both
branches (watch and non-watch) and then either rethrow or continue as
appropriate.
- Around line 378-389: writeTriggerLog is currently awaited before any
seen/failure bookkeeping so if it throws the already-run trigger will be
requeued; wrap the writeTriggerLog(options.projectRoot, detection, result, now)
call in a try/catch that logs the write error (but does not rethrow) and allows
execution to continue to the existing handling of result (including
options.seenStore.mark and markFailure). Ensure the catch references the same
detection/result variables so behavior is unchanged on success and that failures
to write the log are only recorded/logged and do not block seenStore.mark or
failed/cappedFailures increments.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 37a6c8be-04bb-4204-967f-f7c815ac9cfd
📒 Files selected for processing (2)
apps/simplefin-sync/src/trigger.test.tsapps/simplefin-sync/src/trigger.ts
| const result = await trigger(detection); | ||
| await writeTriggerLog(options.projectRoot, detection, result, now); | ||
| if (result.status === "triggered") { | ||
| await options.seenStore.mark(detection.transactionKey); | ||
| triggered += 1; | ||
| } else { | ||
| const failures = await markFailure(options.seenStore, detection.transactionKey); | ||
| if (maxFailures > 0 && failures >= maxFailures) { | ||
| await options.seenStore.mark(detection.transactionKey); | ||
| cappedFailures += 1; | ||
| } | ||
| failed += 1; |
There was a problem hiding this comment.
Don't let log-write failures requeue an already-run trigger.
writeTriggerLog(...) runs before any seen/failure bookkeeping. If the subprocess already completed and the log write fails, this transaction stays eligible and the next poll will run the same trigger again; on the failure path it also prevents maxFailures from ever advancing.
Suggested direction
for (const detection of detections) {
@@
const result = await trigger(detection);
- await writeTriggerLog(options.projectRoot, detection, result, now);
if (result.status === "triggered") {
await options.seenStore.mark(detection.transactionKey);
triggered += 1;
} else {
const failures = await markFailure(options.seenStore, detection.transactionKey);
if (maxFailures > 0 && failures >= maxFailures) {
await options.seenStore.mark(detection.transactionKey);
cappedFailures += 1;
}
failed += 1;
}
+ await writeTriggerLog(options.projectRoot, detection, result, now);
}If the run log is best-effort rather than part of the dedupe contract, catching/logging the write failure is even safer than letting it abort the poll after the subprocess has already run.
Based on learnings, log-before-state ordering is only required when the persisted state stores log_path; this seen-store does not persist the run-log path.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/simplefin-sync/src/trigger.ts` around lines 378 - 389, writeTriggerLog
is currently awaited before any seen/failure bookkeeping so if it throws the
already-run trigger will be requeued; wrap the
writeTriggerLog(options.projectRoot, detection, result, now) call in a try/catch
that logs the write error (but does not rethrow) and allows execution to
continue to the existing handling of result (including options.seenStore.mark
and markFailure). Ensure the catch references the same detection/result
variables so behavior is unchanged on success and that failures to write the log
are only recorded/logged and do not block seenStore.mark or
failed/cappedFailures increments.
1 participant
Summary
Test Plan
bun run typecheckinapps/simplefin-syncbun testinapps/simplefin-syncuv run ruff check .uv run ruff format --check .uv run mypy src/uv run mypy buy_ticket_agentuv run pytest tests/python/test_buy_ticket_agent_smoke.py -q --no-covuv run pytest -q -m "not integration"uv run python -m buy_ticket_agent.main --smokeReview Checklist
Summary by CodeRabbit
New Features
Tests