feat(procurement-approval): migrate approval agent to v0.6 template layout#6841
Open
VasuBansal7576 wants to merge 4 commits intoaden-hive:mainfrom
Open
feat(procurement-approval): migrate approval agent to v0.6 template layout#6841VasuBansal7576 wants to merge 4 commits intoaden-hive:mainfrom
VasuBansal7576 wants to merge 4 commits intoaden-hive:mainfrom
Conversation
📝 WalkthroughWalkthroughAdds a new Procurement Approval Agent template: implementation, CLI, monitor, QuickBooks client and credential resolution, node specs, demos, deploy helpers, tests, and comprehensive README documenting modes and runtime behavior. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Monitor as RequestMonitor
participant Agent as ProcurementApprovalAgent
participant Graph as WorkflowGraph
participant DB as BudgetDB
participant QB as QuickBooks/CSV
User->>Monitor: Drop request JSON into watch dir
Monitor->>Monitor: Move to processing, validate, dedupe (24h)
Monitor->>Agent: run(context)
Agent->>Agent: Ensure setup / choose entry node
Agent->>Graph: Execute nodes (intake → checks → approval → PO)
Graph->>DB: Budget check (SQLite)
Graph->>Agent: Manager approval (HITL)
Agent->>QB: Route sync (API or CSV export)
QB-->>Agent: Return PO ID or CSV artifacts
Agent-->>Monitor: ExecutionResult (success/error + artifacts)
Monitor->>Monitor: Archive request → done/failed, record hash, write result.json
Monitor->>User: Optional notifications (Slack/SMTP) and reveal artifacts
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 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 unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (13)
examples/templates/procurement_approval_agent/deploy/install_launchd.sh (1)
10-11: Consider checking if launchd service is already running before load.The
launchctl load -wmay fail if the service fails to start due to invalid paths. Adding a status check after loading would help diagnose issues.🛠️ Add status verification
launchctl unload "$PLIST_DST" 2>/dev/null || true launchctl load -w "$PLIST_DST" echo "Installed launchd service: $PLIST_DST" echo "Log: /tmp/procurement_approval_agent_launchd.log" +echo "" +echo "Verify status with: launchctl list | grep procurement-approval"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/templates/procurement_approval_agent/deploy/install_launchd.sh` around lines 10 - 11, The script currently runs launchctl unload "$PLIST_DST" and launchctl load -w "$PLIST_DST" without verifying service state; after calling launchctl load -w "$PLIST_DST" (and/or instead of blind load), run a status check using launchctl list for the target label or check the exit status and log output to detect failures (e.g., invalid paths), and if the service failed to start, print an informative error message including the launchctl list output and the PLIST_DST variable so the caller can diagnose the issue; reference the existing launchctl unload/load commands and the PLIST_DST variable when adding this verification.examples/templates/procurement_approval_agent/tests/test_e2e.py (1)
6-17: Consider using set comparison for more concise assertions.The individual
assertstatements work but produce less informative failure messages when multiple nodes are missing. A set comparison would show all missing nodes at once.♻️ Optional: Use set comparison for clearer failure output
def test_graph_has_required_nodes() -> None: node_ids = {node.id for node in default_agent.nodes} - assert "setup-wizard" in node_ids - assert "intake" in node_ids - assert "budget-check" in node_ids - assert "manager-approval" in node_ids - assert "vendor-check" in node_ids - assert "po-generator" in node_ids - assert "integration-check" in node_ids - assert "quickbooks-sync" in node_ids - assert "csv-export" in node_ids - assert "notifications" in node_ids + required = { + "setup-wizard", + "intake", + "budget-check", + "manager-approval", + "vendor-check", + "po-generator", + "integration-check", + "quickbooks-sync", + "csv-export", + "notifications", + } + missing = required - node_ids + assert not missing, f"Missing required nodes: {missing}"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/templates/procurement_approval_agent/tests/test_e2e.py` around lines 6 - 17, The test_graph_has_required_nodes test iterates many separate asserts which hides which nodes are missing; replace the multiple per-node asserts with a single set comparison: build an expected_nodes set of the required ids (e.g., "setup-wizard", "intake", ..., "notifications"), get node_ids = {node.id for node in default_agent.nodes}, then assert that expected_nodes is a subset of node_ids (or assert equality if extras are not allowed) so a failing assertion shows all missing items at once; update the assertion in test_graph_has_required_nodes accordingly.examples/templates/procurement_approval_agent/deploy/com.hive.procurement-approval-agent.plist (2)
18-18: Hardcoded--mockflag enables mock mode by default.The
--mockflag runs with mock LLM responses, which is appropriate for demos but not production. If this plist is intended as a production template, consider removing this flag or documenting that it should be removed for real deployments.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/templates/procurement_approval_agent/deploy/com.hive.procurement-approval-agent.plist` at line 18, The plist currently includes a hardcoded ProgramArguments entry "<string>--mock</string>" which forces mock LLM responses; remove that "<string>--mock</string>" entry from the ProgramArguments array (or replace it with a placeholder/comment and document that it must be omitted for production) so the agent no longer starts in mock mode by default; alternatively make mock mode controlled via an environment variable (e.g., add an EnvironmentVariables key like MOCK_MODE and have the agent read it) and remove the hardcoded "--mock" argument.
1-38: Consider documenting this as an example template.Given that the CLI provides
write-launchdfor generating properly configured plists (seemonitor.py), this static file's role should be clarified. Adding a header comment would help:<!-- EXAMPLE TEMPLATE - DO NOT USE DIRECTLY Generate a properly configured plist using: python -m procurement_approval_agent write-launchd --watch-dir /your/path Or edit the placeholder values below: - /path/to/hive → your repository root - /watched_requests → your request intake directory - Remove --mock for production use -->🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/templates/procurement_approval_agent/deploy/com.hive.procurement-approval-agent.plist` around lines 1 - 38, Add a top-of-file XML comment marking this plist as an "EXAMPLE TEMPLATE - DO NOT USE DIRECTLY" and show how to generate a proper plist with the module CLI (python -m procurement_approval_agent write-launchd), then call out which placeholder values to replace (WorkingDirectory `/path/to/hive` → repo root, the ProgramArguments `--watch-dir /watched_requests` → your intake dir, and to remove the `--mock` argument for production). Also include a short usage snippet referencing the monitor entry point (procurement_approval_agent monitor) so users know this is a sample and should be generated/edited via monitor.py's write-launchd flow.examples/templates/procurement_approval_agent/monitor.py (3)
374-381: SMTP TLS may be required even without authentication.The
starttls()call (line 377) is only invoked when username and password are present. However, many SMTP servers require TLS for all connections, not just authenticated ones. Consider making TLS opt-out rather than opt-in.♻️ Proposed fix to always attempt TLS upgrade
try: with smtplib.SMTP(host, int(port), timeout=10) as smtp: + try: + smtp.starttls() + except smtplib.SMTPNotSupportedError: + pass # Server doesn't support STARTTLS if username and password: - smtp.starttls() smtp.login(username, password) smtp.send_message(msg) except Exception: pass🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/templates/procurement_approval_agent/monitor.py` around lines 374 - 381, The try/except block in monitor.py currently calls smtp.starttls() only when username and password are set, but TLS is often required regardless of auth; change the logic in the block that opens smtplib.SMTP(host, int(port), timeout=10) so it attempts an opportunistic TLS upgrade by calling smtp.starttls() unconditionally (or controlled by a new boolean flag like require_tls) before smtp.login, and only skip starttls if an explicit opt-out is set; update use of username/password so smtp.login(username, password) still only runs when both are present and preserve the existing smtp.send_message(msg) and exception handling.
472-476: HardcodedPYTHONPATHassumes specific repository structure.The launchd plist sets
PYTHONPATHto"core:examples/templates"(line 475), which assumes the monitor runs from the repository root with this exact directory structure. This will break if the template is installed elsewhere or the repo structure changes.Consider making this configurable or deriving it from the working directory:
♻️ Proposed fix to make PYTHONPATH configurable
def launchd_plist_content( label: str, working_dir: Path, watch_dir: Path, log_file: Path, poll_interval: float, + python_path: str = "core:examples/templates", ) -> str: """Generate launchd plist content for macOS background service.""" return f"""<?xml version=\"1.0\" encoding=\"UTF-8\"?> ... <key>EnvironmentVariables</key> <dict> <key>PYTHONPATH</key> - <string>core:examples/templates</string> + <string>{python_path}</string> </dict> ...🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/templates/procurement_approval_agent/monitor.py` around lines 472 - 476, The launchd plist hardcodes PYTHONPATH ("core:examples/templates") under EnvironmentVariables which assumes a fixed repo layout; change this to compute or configure the path at runtime instead: add code in monitor.py that derives the correct PYTHONPATH from the script's location or an env/config value and injects that computed value into the plist generation (replace the hardcoded PYTHONPATH entry), or expose a configurable parameter so EnvironmentVariables.PYTHONPATH is set to the resolved path or provided value before writing the plist.
352-355: Silent exception swallowing hides Slack notification failures.The bare
except Exception: passon lines 354-355 makes debugging configuration issues difficult. Consider logging the exception at debug level.♻️ Proposed fix to log notification failures
try: urllib.request.urlopen(req, timeout=5).read() - except Exception: - pass + except Exception as exc: + print(f"[monitor] Slack notification failed: {exc}")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/templates/procurement_approval_agent/monitor.py` around lines 352 - 355, The try/except around urllib.request.urlopen(req, timeout=5).read() is silently swallowing all exceptions; modify the except to capture the exception (e.g., "except Exception as e") and log the failure at debug (or warn) level including the exception message and context (mentioning req/Slack notification) so notification failures are visible; use the module's existing logger instance (refer to the logger used elsewhere in monitor.py) when logging.examples/templates/procurement_approval_agent/__main__.py (2)
106-111: Redundant credential check; simplify to single call.Line 107 calls
has_quickbooks_api_credentials()withoutcredential_ref, then lines 109-111 conditionally call it again withcredential_ref. The second call should always be used since it handles both cases.♻️ Proposed fix to consolidate credential checks
if process: - default_has_qb = has_quickbooks_api_credentials() - if qb_credential_ref: - default_has_qb = has_quickbooks_api_credentials( - credential_ref=qb_credential_ref - ) + default_has_qb = has_quickbooks_api_credentials( + credential_ref=qb_credential_ref + ) has_qb = click.confirm(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/templates/procurement_approval_agent/__main__.py` around lines 106 - 111, The code redundantly calls has_quickbooks_api_credentials() twice; replace the two-step logic in the block that sets default_has_qb (inside the if process: branch) with a single call that always passes qb_credential_ref (which can be None) to has_quickbooks_api_credentials — i.e., remove the initial default_has_qb=has_quickbooks_api_credentials() and instead set default_has_qb = has_quickbooks_api_credentials(credential_ref=qb_credential_ref) so the function handles both the presence and absence of qb_credential_ref.
197-197: Default--watch-dirpath requires root permissions.The default
/watched_requestsis a root-level absolute path that typically requires elevated permissions on Unix systems. Consider using a user-writable default like~/watched_requestsor a relative path.♻️ Proposed fix to use user-writable default
-@click.option("--watch-dir", default="/watched_requests", show_default=True) +@click.option("--watch-dir", default="~/watched_requests", show_default=True)The same applies to line 331 for the
write-launchdcommand.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/templates/procurement_approval_agent/__main__.py` at line 197, Change the hard-coded root-level default for the Click option "--watch-dir" to a user-writable path and ensure expansion is handled; update the Click option definition(s) that set default="/watched_requests" (the one decorating the main/watch CLI function and the one used by the "write-launchd" command) to use a safer default such as "~/watched_requests" or "watched_requests", and make sure the code that consumes the option calls os.path.expanduser on the value before using it so tilde expansion works for users.examples/templates/procurement_approval_agent/agent.py (1)
293-300: Broadexceptclause may hide unexpected errors.The bare
except Exceptionon line 299 catches all exceptions when loading setup state, which could mask issues like permission errors or disk failures.♻️ Proposed fix to catch specific exceptions
try: raw = json.loads(path.read_text(encoding="utf-8")) return { "setup_completed": bool(raw.get("setup_completed", False)), "preferred_sync_method": raw.get("preferred_sync_method"), } - except Exception: + except (json.JSONDecodeError, OSError, KeyError, TypeError): return {"setup_completed": False, "preferred_sync_method": None}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/templates/procurement_approval_agent/agent.py` around lines 293 - 300, The current bare except around json.loads(path.read_text(...)) swallows all errors; update the setup-state load block to catch and handle specific exceptions instead: catch json.JSONDecodeError to return {"setup_completed": False, "preferred_sync_method": None} for corrupt JSON, catch FileNotFoundError to return the same default, and catch permission/IO issues via PermissionError/OSError and either log and re-raise or return the default depending on desired behavior; reference the json.loads call, path.read_text, and the returned keys ("setup_completed" and "preferred_sync_method") when making the changes.examples/templates/procurement_approval_agent/nodes/quickbooks.py (1)
96-103: PotentialValueErrorifoutput_diris outside the package directory.The
relative_to()call on lines 99 and 101 will raiseValueErrorifoutput_diris an absolute path outside ofPath(__file__).resolve().parents[1]. This could occur if a caller passes an arbitrary temporary directory.Consider handling this edge case or documenting the constraint:
♻️ Proposed fix to handle paths outside the package
print(f"[mock-qb-csv] generated fallback CSV for PO {po_number}") + package_root = Path(__file__).resolve().parents[1] + try: + csv_rel = str(csv_path.relative_to(package_root)) + md_rel = str(md_path.relative_to(package_root)) + except ValueError: + csv_rel = str(csv_path) + md_rel = str(md_path) return { - "csv_file_path": str(csv_path.relative_to(Path(__file__).resolve().parents[1])), - "import_instructions": str( - md_path.relative_to(Path(__file__).resolve().parents[1]) - ), + "csv_file_path": csv_rel, + "import_instructions": md_rel, }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/templates/procurement_approval_agent/nodes/quickbooks.py` around lines 96 - 103, The code returns csv_file_path and import_instructions using csv_path.relative_to(Path(__file__).resolve().parents[1]) which will raise ValueError when the generated csv_path or md_path is outside the package root (e.g., caller passes a tmp dir); update the return logic in the function that creates these paths (look for csv_path and md_path in quickbooks.py) to catch that case and fall back to a safe alternative such as using os.path.relpath(csv_path, Path(__file__).resolve().parents[1]) or returning the absolute path when relative_to would fail (wrap the relative_to calls in a try/except ValueError and return a sensible absolute path or relpath fallback for both csv_path and md_path).examples/templates/procurement_approval_agent/quickbooks_api.py (1)
177-193: 401 handling delays retry unnecessarily after token refresh.After a 401 response, the code refreshes the token (line 180) but then falls through to the sleep on line 193 before the next retry attempt. This adds unnecessary delay since the token was just refreshed.
♻️ Proposed fix to retry immediately after 401 token refresh
except urllib.error.HTTPError as exc: # One token refresh retry path on auth failure. if exc.code == 401: self._refresh_access_token(force=True) + last_error = exc + continue # Retry immediately with new token elif exc.code in (429, 500, 502, 503, 504): pass else: detail = exc.read().decode("utf-8", errors="ignore") raise QuickBooksAPIError( f"QuickBooks API HTTP {exc.code}: {detail or exc.reason}" ) from exc last_error = exc except (urllib.error.URLError, TimeoutError) as exc: last_error = exc if attempt < retries - 1: time.sleep(base_delay * (2**attempt))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/templates/procurement_approval_agent/quickbooks_api.py` around lines 177 - 193, The 401 branch refreshes the token via self._refresh_access_token(force=True) but then still triggers the exponential backoff sleep before retry; change the 401 handling in the except urllib.error.HTTPError block (the branch that calls self._refresh_access_token) so that after refreshing you set last_error = exc and immediately continue the retry loop (skip the sleep) rather than allowing the flow to reach the sleep at the end of the iteration; implement this by returning to the loop (e.g., continue) right after the refresh (or using a refreshed flag checked before sleeping) in the method that performs the HTTP retries.examples/templates/procurement_approval_agent/tests/test_basic_workflow.py (1)
139-145: Fragile path manipulation withreplace("data/", "").The path assertions on lines 140-145 use string replacement which could fail if the path format changes or contains "data/" elsewhere in the path.
♻️ Proposed fix to use proper path manipulation
data_dir = Path(__file__).resolve().parents[1] / "data" - assert ( - data_dir / output["csv_file_path"].replace("data/", "") - ).exists() is True - assert ( - data_dir / output["import_instructions"].replace("data/", "") - ).exists() is True + # csv_file_path format: "data/po/PO-xxx_qb_manual_import.csv" + csv_path = Path(output["csv_file_path"]) + if csv_path.parts[0] == "data": + csv_path = Path(*csv_path.parts[1:]) + assert (data_dir / csv_path).exists() is True + + md_path = Path(output["import_instructions"]) + if md_path.parts[0] == "data": + md_path = Path(*md_path.parts[1:]) + assert (data_dir / md_path).exists() is True🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/templates/procurement_approval_agent/tests/test_basic_workflow.py` around lines 139 - 145, The assertions are fragile because they use string replace("data/", "") on output paths; instead convert the output values to Path objects and join with data_dir using proper path methods: for both output["csv_file_path"] and output["import_instructions"] use Path(output["..."]) to extract a safe relative fragment (e.g., .name or .relative_to("data") if guaranteed) and then assert (data_dir / that_path).exists(); update the assertions around the data_dir variable and output[...] references accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@examples/templates/procurement_approval_agent/agent.py`:
- Line 404: The po_number variable is hardcoded as "PO-20260224-001" which
causes duplicate artifacts; update the assignment to generate a unique PO per
run (e.g., include a timestamp or UUID) where po_number is set in agent.py so
each workflow produces a distinct identifier; ensure the generated format still
matches downstream expectations (prefix "PO-" + unique suffix) and adjust any
downstream validation or QuickBooks DocNumber usage to accept the new format.
In `@examples/templates/procurement_approval_agent/credentials.py`:
- Around line 53-54: The code eagerly defaults environment to "sandbox" which
prevents credential-store values from taking effect; change the logic in
credentials.py to initialize environment without a default (e.g., environment =
os.environ.get("QUICKBOOKS_ENV") or environment = None), then after
resolving/reading the credential store (use the same code paths that reference
the credential resolution functions) assign environment = store_value if
present, and only if environment is still None set the fallback to "sandbox";
apply the same pattern to the other similar occurrences (the blocks around the
usages referenced at 124-127 and 129-135) so credential-derived environment
values can override the default.
In `@examples/templates/procurement_approval_agent/demo.sh`:
- Line 120: The one-liner uses a broad pkill -f "procurement_approval_agent
monitor" which can terminate unrelated processes; instead, scope termination by
tracking the exact PID for the instance you start (create and use a PID file
when launching the monitor) or, if you can't change startup, use pgrep with
additional filters (e.g., pgrep -u "$USER" -f "procurement_approval_agent
monitor" and verify command/working-dir/parent PID) to resolve the single PID
and kill it, then remove the PID file; update the invocation that currently runs
pkill -f "procurement_approval_agent monitor" to use the PID-file or filtered
pgrep approach and fall back safely (no-op) if no matching PID is found.
In
`@examples/templates/procurement_approval_agent/deploy/com.hive.procurement-approval-agent.plist`:
- Around line 15-21: This plist contains placeholder values that will break on
real systems; update the file so it is clearly a template or usable: either
rename the file to include ".example" (e.g., change
com.hive.procurement-approval-agent.plist to
com.hive.procurement-approval-agent.plist.example), or add a top-of-file comment
block explaining that the --watch-dir string "/watched_requests" and the
WorkingDirectory value "/path/to/hive" must be customized before use, and
optionally replace those values with macOS-supported environment variable
references; also reference the dynamic generator function
launchd_plist_content() in monitor.py in the comment so consumers know they can
programmatically generate a correct plist.
In `@examples/templates/procurement_approval_agent/deploy/install_launchd.sh`:
- Around line 4-8: The script currently copies a static PLIST_SRC which contains
placeholder paths and will break launchd; replace this by invoking the dynamic
plist generator instead: call the CLI command that triggers
monitor.write_launchd_plist() (e.g., the project's "write-launchd" CLI entry) or
import and call write_launchd_plist() to create PLIST_DST with real paths,
ensuring PLIST_SRC/PLIST_DST are set to the generated file; alternatively, if
you keep a copy step, add a prominent interactive prompt or warning to require
users to edit placeholders before installation (refer to PLIST_SRC, PLIST_DST,
write_launchd_plist(), and the "write-launchd" CLI).
In `@examples/templates/procurement_approval_agent/flowchart.json`:
- Around line 29-31: The flowchart start metadata conflicts with the runtime
entry node: "setup-wizard" is marked as the start via the
"flowchart_type"/"flowchart_shape"/"flowchart_color" block while "entry_node" is
set to "pre-execution-check"; update either the entry_node value to
"setup-wizard" or change the start-node metadata to "pre-execution-check" so
both the flowchart start and runtime entry point match (also apply the same fix
to the other occurrence around the second block noted).
In `@examples/templates/procurement_approval_agent/quickbooks_api.py`:
- Around line 117-139: The AccountBasedExpenseLineDetail in
_build_purchase_order_payload is empty causing QuickBooks validation errors;
populate it with an AccountRef taken from po_data (e.g.,
po_data.get("account_id") or a sensible default) so the returned payload's
"Line"[0]["AccountBasedExpenseLineDetail"] includes "AccountRef": {"value":
"<account_id>"} and ensure the code converts the account id to a string if
needed.
In `@examples/templates/procurement_approval_agent/README.md`:
- Around line 37-44: Update the inconsistent wording that says "three env vars"
while the block lists four QuickBooks environment variables; change the sentence
to "Set all four env vars to enable API mode:" (ensuring the listed variables
QUICKBOOKS_CLIENT_ID, QUICKBOOKS_CLIENT_SECRET, QUICKBOOKS_REALM_ID, and
QUICKBOOKS_REFRESH_TOKEN remain unchanged).
---
Nitpick comments:
In `@examples/templates/procurement_approval_agent/__main__.py`:
- Around line 106-111: The code redundantly calls
has_quickbooks_api_credentials() twice; replace the two-step logic in the block
that sets default_has_qb (inside the if process: branch) with a single call that
always passes qb_credential_ref (which can be None) to
has_quickbooks_api_credentials — i.e., remove the initial
default_has_qb=has_quickbooks_api_credentials() and instead set default_has_qb =
has_quickbooks_api_credentials(credential_ref=qb_credential_ref) so the function
handles both the presence and absence of qb_credential_ref.
- Line 197: Change the hard-coded root-level default for the Click option
"--watch-dir" to a user-writable path and ensure expansion is handled; update
the Click option definition(s) that set default="/watched_requests" (the one
decorating the main/watch CLI function and the one used by the "write-launchd"
command) to use a safer default such as "~/watched_requests" or
"watched_requests", and make sure the code that consumes the option calls
os.path.expanduser on the value before using it so tilde expansion works for
users.
In `@examples/templates/procurement_approval_agent/agent.py`:
- Around line 293-300: The current bare except around
json.loads(path.read_text(...)) swallows all errors; update the setup-state load
block to catch and handle specific exceptions instead: catch
json.JSONDecodeError to return {"setup_completed": False,
"preferred_sync_method": None} for corrupt JSON, catch FileNotFoundError to
return the same default, and catch permission/IO issues via
PermissionError/OSError and either log and re-raise or return the default
depending on desired behavior; reference the json.loads call, path.read_text,
and the returned keys ("setup_completed" and "preferred_sync_method") when
making the changes.
In
`@examples/templates/procurement_approval_agent/deploy/com.hive.procurement-approval-agent.plist`:
- Line 18: The plist currently includes a hardcoded ProgramArguments entry
"<string>--mock</string>" which forces mock LLM responses; remove that
"<string>--mock</string>" entry from the ProgramArguments array (or replace it
with a placeholder/comment and document that it must be omitted for production)
so the agent no longer starts in mock mode by default; alternatively make mock
mode controlled via an environment variable (e.g., add an EnvironmentVariables
key like MOCK_MODE and have the agent read it) and remove the hardcoded "--mock"
argument.
- Around line 1-38: Add a top-of-file XML comment marking this plist as an
"EXAMPLE TEMPLATE - DO NOT USE DIRECTLY" and show how to generate a proper plist
with the module CLI (python -m procurement_approval_agent write-launchd), then
call out which placeholder values to replace (WorkingDirectory `/path/to/hive` →
repo root, the ProgramArguments `--watch-dir /watched_requests` → your intake
dir, and to remove the `--mock` argument for production). Also include a short
usage snippet referencing the monitor entry point (procurement_approval_agent
monitor) so users know this is a sample and should be generated/edited via
monitor.py's write-launchd flow.
In `@examples/templates/procurement_approval_agent/deploy/install_launchd.sh`:
- Around line 10-11: The script currently runs launchctl unload "$PLIST_DST" and
launchctl load -w "$PLIST_DST" without verifying service state; after calling
launchctl load -w "$PLIST_DST" (and/or instead of blind load), run a status
check using launchctl list for the target label or check the exit status and log
output to detect failures (e.g., invalid paths), and if the service failed to
start, print an informative error message including the launchctl list output
and the PLIST_DST variable so the caller can diagnose the issue; reference the
existing launchctl unload/load commands and the PLIST_DST variable when adding
this verification.
In `@examples/templates/procurement_approval_agent/monitor.py`:
- Around line 374-381: The try/except block in monitor.py currently calls
smtp.starttls() only when username and password are set, but TLS is often
required regardless of auth; change the logic in the block that opens
smtplib.SMTP(host, int(port), timeout=10) so it attempts an opportunistic TLS
upgrade by calling smtp.starttls() unconditionally (or controlled by a new
boolean flag like require_tls) before smtp.login, and only skip starttls if an
explicit opt-out is set; update use of username/password so smtp.login(username,
password) still only runs when both are present and preserve the existing
smtp.send_message(msg) and exception handling.
- Around line 472-476: The launchd plist hardcodes PYTHONPATH
("core:examples/templates") under EnvironmentVariables which assumes a fixed
repo layout; change this to compute or configure the path at runtime instead:
add code in monitor.py that derives the correct PYTHONPATH from the script's
location or an env/config value and injects that computed value into the plist
generation (replace the hardcoded PYTHONPATH entry), or expose a configurable
parameter so EnvironmentVariables.PYTHONPATH is set to the resolved path or
provided value before writing the plist.
- Around line 352-355: The try/except around urllib.request.urlopen(req,
timeout=5).read() is silently swallowing all exceptions; modify the except to
capture the exception (e.g., "except Exception as e") and log the failure at
debug (or warn) level including the exception message and context (mentioning
req/Slack notification) so notification failures are visible; use the module's
existing logger instance (refer to the logger used elsewhere in monitor.py) when
logging.
In `@examples/templates/procurement_approval_agent/nodes/quickbooks.py`:
- Around line 96-103: The code returns csv_file_path and import_instructions
using csv_path.relative_to(Path(__file__).resolve().parents[1]) which will raise
ValueError when the generated csv_path or md_path is outside the package root
(e.g., caller passes a tmp dir); update the return logic in the function that
creates these paths (look for csv_path and md_path in quickbooks.py) to catch
that case and fall back to a safe alternative such as using
os.path.relpath(csv_path, Path(__file__).resolve().parents[1]) or returning the
absolute path when relative_to would fail (wrap the relative_to calls in a
try/except ValueError and return a sensible absolute path or relpath fallback
for both csv_path and md_path).
In `@examples/templates/procurement_approval_agent/quickbooks_api.py`:
- Around line 177-193: The 401 branch refreshes the token via
self._refresh_access_token(force=True) but then still triggers the exponential
backoff sleep before retry; change the 401 handling in the except
urllib.error.HTTPError block (the branch that calls self._refresh_access_token)
so that after refreshing you set last_error = exc and immediately continue the
retry loop (skip the sleep) rather than allowing the flow to reach the sleep at
the end of the iteration; implement this by returning to the loop (e.g.,
continue) right after the refresh (or using a refreshed flag checked before
sleeping) in the method that performs the HTTP retries.
In `@examples/templates/procurement_approval_agent/tests/test_basic_workflow.py`:
- Around line 139-145: The assertions are fragile because they use string
replace("data/", "") on output paths; instead convert the output values to Path
objects and join with data_dir using proper path methods: for both
output["csv_file_path"] and output["import_instructions"] use
Path(output["..."]) to extract a safe relative fragment (e.g., .name or
.relative_to("data") if guaranteed) and then assert (data_dir /
that_path).exists(); update the assertions around the data_dir variable and
output[...] references accordingly.
In `@examples/templates/procurement_approval_agent/tests/test_e2e.py`:
- Around line 6-17: The test_graph_has_required_nodes test iterates many
separate asserts which hides which nodes are missing; replace the multiple
per-node asserts with a single set comparison: build an expected_nodes set of
the required ids (e.g., "setup-wizard", "intake", ..., "notifications"), get
node_ids = {node.id for node in default_agent.nodes}, then assert that
expected_nodes is a subset of node_ids (or assert equality if extras are not
allowed) so a failing assertion shows all missing items at once; update the
assertion in test_graph_has_required_nodes accordingly.
🪄 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: 49c1b455-9cd6-45df-8e91-c52e243d8d7d
⛔ Files ignored due to path filters (2)
examples/templates/procurement_approval_agent/data/approved_vendors.csvis excluded by!**/*.csvexamples/templates/procurement_approval_agent/data/budget_tracking.dbis excluded by!**/*.db
📒 Files selected for processing (24)
examples/templates/procurement_approval_agent/README.mdexamples/templates/procurement_approval_agent/__init__.pyexamples/templates/procurement_approval_agent/__main__.pyexamples/templates/procurement_approval_agent/agent.pyexamples/templates/procurement_approval_agent/config.pyexamples/templates/procurement_approval_agent/credentials.pyexamples/templates/procurement_approval_agent/demo.shexamples/templates/procurement_approval_agent/demo_workflows.shexamples/templates/procurement_approval_agent/deploy/com.hive.procurement-approval-agent.plistexamples/templates/procurement_approval_agent/deploy/install_launchd.shexamples/templates/procurement_approval_agent/flowchart.jsonexamples/templates/procurement_approval_agent/mcp_servers.jsonexamples/templates/procurement_approval_agent/monitor.pyexamples/templates/procurement_approval_agent/nodes/__init__.pyexamples/templates/procurement_approval_agent/nodes/approval.pyexamples/templates/procurement_approval_agent/nodes/budget_checker.pyexamples/templates/procurement_approval_agent/nodes/intake.pyexamples/templates/procurement_approval_agent/nodes/po_generator.pyexamples/templates/procurement_approval_agent/nodes/quickbooks.pyexamples/templates/procurement_approval_agent/quickbooks_api.pyexamples/templates/procurement_approval_agent/tests/test_basic_workflow.pyexamples/templates/procurement_approval_agent/tests/test_cli.pyexamples/templates/procurement_approval_agent/tests/test_e2e.pyexamples/templates/procurement_approval_agent/tests/test_monitoring.py
examples/templates/procurement_approval_agent/deploy/com.hive.procurement-approval-agent.plist
Outdated
Show resolved
Hide resolved
examples/templates/procurement_approval_agent/deploy/install_launchd.sh
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@examples/templates/procurement_approval_agent/agent.py`:
- Around line 395-406: The run() implementation is hardcoding approvals
(budget_status="auto_approved", remaining_budget=32000, vendor_approved=True)
and thus bypasses procurement gates; replace those hardcoded assignments with
real checks: after building validated_request call the budget check function
(e.g., check_budget or get_remaining_budget) to set remaining_budget and compute
budget_status (approved/over_budget), call the vendor validation function (e.g.,
validate_vendor or check_vendor) to set vendor_approved, and if either check
fails route to the manager-approval flow (e.g., request_manager_approval)
instead of proceeding to PO generation/sync; update output["budget_status"],
output["remaining_budget"], and output["vendor_approved"] based on those results
and ensure any subsequent logic (generate_po, sync_po) only runs when both
budget_status and vendor_approved indicate approval.
In `@examples/templates/procurement_approval_agent/credentials.py`:
- Around line 56-65: The current early return when client_id, client_secret, and
realm_id are present prevents using effective_ref (credential_ref or
QUICKBOOKS_CREDENTIAL_REF) to backfill missing fields like refresh_token or
environment; update the logic around effective_ref and QuickBooksCredentials
construction so that if credential_ref/effective_ref is provided you first
attempt to load and merge stored credentials (or missing fields) before
returning, and only return immediately if all required fields including
refresh_token and environment are populated; reference the symbols
effective_ref, credential_ref, and the QuickBooksCredentials constructor to
locate and modify the flow.
In `@examples/templates/procurement_approval_agent/quickbooks_api.py`:
- Around line 266-279: The _save_token_cache method currently creates the parent
directory and writes the token file but doesn't enforce restrictive permissions;
update it to set the parent directory to mode 0o700 (use pathlib.Path.mkdir with
mode=0o700 or call os.chmod on token_cache_path.parent) and after writing the
token file set the token file mode to 0o600 (use os.chmod on token_cache_path).
Reference the _save_token_cache method and
token_cache_path/_access_token/_token_expires_at to locate where to create the
directory, write the file, and then apply the secure permissions.
In `@examples/templates/procurement_approval_agent/tests/test_basic_workflow.py`:
- Around line 15-50: The _setup/test_data function mutates the repo-level data/
tree; change it to create all test artifacts in an isolated temporary directory
(use pytest's tmp_path fixture or tempfile.TemporaryDirectory) instead of using
agent_dir / "data", write budget_tracking.db, approved_vendors.csv and
qb_mock_responses.json into that temp dir, and remove any logic that unlinks the
checked-in qb_mock_responses.json so the repo is not modified; update callers of
_setup_test_data to accept or use the temp path (referenced symbols:
_setup_test_data, data_dir, db_path, vendors_csv, qb_mock_path).
🪄 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: de57f2fb-3b58-4f18-911f-eb2dd9af19cb
📒 Files selected for processing (10)
examples/templates/procurement_approval_agent/README.mdexamples/templates/procurement_approval_agent/agent.pyexamples/templates/procurement_approval_agent/credentials.pyexamples/templates/procurement_approval_agent/demo.shexamples/templates/procurement_approval_agent/demo_workflows.shexamples/templates/procurement_approval_agent/deploy/install_launchd.shexamples/templates/procurement_approval_agent/flowchart.jsonexamples/templates/procurement_approval_agent/quickbooks_api.pyexamples/templates/procurement_approval_agent/tests/test_basic_workflow.pyexamples/templates/procurement_approval_agent/tests/test_e2e.py
✅ Files skipped from review due to trivial changes (1)
- examples/templates/procurement_approval_agent/flowchart.json
🚧 Files skipped from review as they are similar to previous changes (4)
- examples/templates/procurement_approval_agent/deploy/install_launchd.sh
- examples/templates/procurement_approval_agent/tests/test_e2e.py
- examples/templates/procurement_approval_agent/demo_workflows.sh
- examples/templates/procurement_approval_agent/demo.sh
examples/templates/procurement_approval_agent/tests/test_basic_workflow.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (6)
examples/templates/procurement_approval_agent/agent.py (4)
527-542: Consider broader exception handling for QuickBooks integration.Only
QuickBooksAPIErroris caught, so unexpected exceptions (e.g., network timeouts, JSON decode errors, or missing token file errors fromfrom_env) will propagate uncaught and bypass theExecutionResultreturn path. If you want consistent error reporting regardless of failure type, consider catching a broader exception class as a fallback.♻️ Proposed enhancement
except QuickBooksAPIError as exc: output["sync_status"] = "api_error" output["sync_error"] = str(exc) return ExecutionResult( success=False, output=output, error=str(exc), steps_executed=10 ) + except Exception as exc: + output["sync_status"] = "unexpected_error" + output["sync_error"] = str(exc) + return ExecutionResult( + success=False, output=output, error=f"Unexpected QuickBooks error: {exc}", steps_executed=10 + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/templates/procurement_approval_agent/agent.py` around lines 527 - 542, The QuickBooks block currently only catches QuickBooksAPIError so other failures (from QuickBooksAPI.from_env, qb_api.create_purchase_order, file I/O, network, JSON, etc.) will escape; add a fallback broad exception handler (catch Exception) around the same try/except that references token_cache / QuickBooksAPI.from_env and qb_api.create_purchase_order, and in that handler set output["sync_status"]="api_error" (or a distinct value like "unexpected_error"), set output["sync_error"]=str(exc), and return an ExecutionResult(success=False, output=output, error=str(exc), steps_executed=10) to ensure consistent error reporting for all failure types while preserving the existing QuickBooksAPIError branch.
571-584: Consider extending validation to cover terminal and pause nodes.The
validate()method checksentry_nodeand edge endpoints but doesn't verify thatterminal_nodesandpause_nodesreference valid node IDs. This could catch configuration errors earlier.♻️ Proposed enhancement
def validate(self) -> dict: errors: list[str] = [] node_ids = {n.id for n in self.nodes} if self.entry_node not in node_ids: errors.append(f"entry_node '{self.entry_node}' not in nodes") + for term in self.terminal_nodes: + if term not in node_ids: + errors.append(f"terminal_node '{term}' not in nodes") + + for pause in self.pause_nodes: + if pause not in node_ids: + errors.append(f"pause_node '{pause}' not in nodes") + for edge in self.edges: if edge.source not in node_ids: errors.append(f"edge {edge.id} has unknown source '{edge.source}'") if edge.target not in node_ids: errors.append(f"edge {edge.id} has unknown target '{edge.target}'") return {"valid": not errors, "errors": errors, "warnings": []}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/templates/procurement_approval_agent/agent.py` around lines 571 - 584, The validate() method currently checks entry_node and edge endpoints but misses verifying terminal_nodes and pause_nodes; update validate (in the class containing nodes, edges, entry_node) to also ensure every id in self.terminal_nodes and self.pause_nodes exists in the node_ids set, appending clear error messages like "terminal_node 'X' not in nodes" or "pause_node 'Y' not in nodes" when missing, and return them inside the existing {"valid": ..., "errors": ..., "warnings": ...} structure so invalid references are caught early.
407-413: Storage path initialization is duplicated.Lines 407-413 duplicate the storage path setup from
_setup()(lines 364-370). Sincerun()callsstart()which calls_setup(), this is redundant. Consider removing the duplication and relying on_setup()to initialize_storage_path.♻️ Proposed refactor
async def run( self, context: dict, mock_mode: bool = False, mock_qb: bool = True ) -> ExecutionResult: - storage_root = Path( - os.environ.get( - "HIVE_AGENT_STORAGE_ROOT", str(Path.home() / ".hive" / "agents") - ) - ) - self._storage_path = storage_root / "procurement_approval_agent" - self._storage_path.mkdir(parents=True, exist_ok=True) - await self.start(mock_mode=mock_mode, mock_qb=mock_qb, run_context=context) + assert self._storage_path is not None # Initialized by _setup via start()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/templates/procurement_approval_agent/agent.py` around lines 407 - 413, The storage path initialization block (creation of storage_root and self._storage_path with mkdir) is duplicated in run(); remove that duplicate block and rely on the existing initialization in _setup() which is invoked via start(); ensure run() still calls start() before any use of self._storage_path (or explicitly call _setup() if the call path changes) so that storage_root, self._storage_path, and its directory are initialized exactly once.
459-470: Manager approval defaults to "approved" when not explicitly set.When
budget_status == "needs_approval", line 460 defaultsapproval_decisionto"approved"if not present in context. This effectively auto-approves requests that exceed the 90% budget threshold when no explicit decision is provided.If this is intentional for mock/test mode orchestration (where the caller pre-populates approval decisions), consider adding a comment clarifying the expected caller contract. Otherwise, for real HITL workflows, this would need to pause execution and await an actual manager decision.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/templates/procurement_approval_agent/agent.py` around lines 459 - 470, The code is auto-approving manager sign-off by defaulting approval_decision to "approved" when budget_status == "needs_approval"; remove that implicit default in the procurement_approval_agent flow (the block that reads/sets approval_decision on the output dict) and instead require an explicit approval decision from the caller or return a pending/failure ExecutionResult (use ExecutionResult(success=False, output=output, error="Manager approval required", steps_executed=6) or a clear "pending approval" signal) so HITL workflows pause for a real manager decision; if the default was intentional for tests, add a clarifying comment above the budget_status == "needs_approval" branch documenting the caller contract.examples/templates/procurement_approval_agent/tests/test_basic_workflow.py (2)
145-150: Path manipulation via string replacement is fragile.Using
.replace("data/", "")to construct file paths couples tests to the exact path format returned by the agent. Consider usingPathoperations or having the agent return absolute paths for artifact verification.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/templates/procurement_approval_agent/tests/test_basic_workflow.py` around lines 145 - 150, The assertions in test_basic_workflow use fragile string replacement on output["csv_file_path"] and output["import_instructions"]; replace this by building paths with pathlib.Path instead of .replace — e.g., convert the returned strings with Path(output["csv_file_path"]) and Path(output["import_instructions"]) and either join their .name with data_dir (data_dir / path.name) or resolve absolute paths (path.resolve()) before calling .exists(); update the assertions that reference data_dir and the output dict to use these Path operations so the test no longer depends on the exact "data/" prefix format.
82-116: Inconsistent environment variable management across tests.Some tests (e.g., lines 82-116) use manual save/restore in try/finally blocks, while others (e.g., lines 193-216) use pytest's
monkeypatch. Consider standardizing onmonkeypatchthroughout for cleaner, less error-prone env var management.Also applies to: 193-216
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/templates/procurement_approval_agent/tests/test_basic_workflow.py` around lines 82 - 116, Replace the manual try/finally environment save/restore using previous_storage_root/previous_data_dir with pytest's monkeypatch fixture: call monkeypatch.setenv("HIVE_AGENT_STORAGE_ROOT", tmpdir) and monkeypatch.setenv("PROCUREMENT_APPROVAL_AGENT_DATA_DIR", str(data_dir)) before invoking _run_workflow(mock_qb=True, ...), remove the try/finally block, and do not manually restore those env vars; keep the assertions (qb_mock_path, qb_records, etc.) and retain _set_qb_creds(enabled=False) in test teardown if needed (or call it directly after assertions) so behavior of _run_workflow and _set_qb_creds remains unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@examples/templates/procurement_approval_agent/tests/test_basic_workflow.py`:
- Around line 61-74: The tests use the module-level singleton default_agent
which preserves its internal _graph after the first start(), causing cross-test
state leakage; fix by ensuring each test uses a fresh agent instance or resets
the singleton before running: either instantiate a new
ProcurementApprovalAgent() and call its run(...) in place of
default_agent.run(), or explicitly set default_agent._graph = None (and other
mutable internals like storage roots if used) in test setup/teardown so start()
will call _setup() again; update all tests that call default_agent.run()
(including other occurrences around lines 276-278) to follow one of these
approaches.
- Around line 119-160: The finally block in
test_full_workflow_csv_fallback_mock_mode is missing a call to
_set_qb_creds(enabled=False) to restore QuickBooks credential state; update the
finally block to call _set_qb_creds(enabled=False) (same as in
test_full_workflow_api_path_mock_mode) so credentials are explicitly disabled
during cleanup and future test changes can't leave creds enabled.
---
Nitpick comments:
In `@examples/templates/procurement_approval_agent/agent.py`:
- Around line 527-542: The QuickBooks block currently only catches
QuickBooksAPIError so other failures (from QuickBooksAPI.from_env,
qb_api.create_purchase_order, file I/O, network, JSON, etc.) will escape; add a
fallback broad exception handler (catch Exception) around the same try/except
that references token_cache / QuickBooksAPI.from_env and
qb_api.create_purchase_order, and in that handler set
output["sync_status"]="api_error" (or a distinct value like "unexpected_error"),
set output["sync_error"]=str(exc), and return an ExecutionResult(success=False,
output=output, error=str(exc), steps_executed=10) to ensure consistent error
reporting for all failure types while preserving the existing QuickBooksAPIError
branch.
- Around line 571-584: The validate() method currently checks entry_node and
edge endpoints but misses verifying terminal_nodes and pause_nodes; update
validate (in the class containing nodes, edges, entry_node) to also ensure every
id in self.terminal_nodes and self.pause_nodes exists in the node_ids set,
appending clear error messages like "terminal_node 'X' not in nodes" or
"pause_node 'Y' not in nodes" when missing, and return them inside the existing
{"valid": ..., "errors": ..., "warnings": ...} structure so invalid references
are caught early.
- Around line 407-413: The storage path initialization block (creation of
storage_root and self._storage_path with mkdir) is duplicated in run(); remove
that duplicate block and rely on the existing initialization in _setup() which
is invoked via start(); ensure run() still calls start() before any use of
self._storage_path (or explicitly call _setup() if the call path changes) so
that storage_root, self._storage_path, and its directory are initialized exactly
once.
- Around line 459-470: The code is auto-approving manager sign-off by defaulting
approval_decision to "approved" when budget_status == "needs_approval"; remove
that implicit default in the procurement_approval_agent flow (the block that
reads/sets approval_decision on the output dict) and instead require an explicit
approval decision from the caller or return a pending/failure ExecutionResult
(use ExecutionResult(success=False, output=output, error="Manager approval
required", steps_executed=6) or a clear "pending approval" signal) so HITL
workflows pause for a real manager decision; if the default was intentional for
tests, add a clarifying comment above the budget_status == "needs_approval"
branch documenting the caller contract.
In `@examples/templates/procurement_approval_agent/tests/test_basic_workflow.py`:
- Around line 145-150: The assertions in test_basic_workflow use fragile string
replacement on output["csv_file_path"] and output["import_instructions"];
replace this by building paths with pathlib.Path instead of .replace — e.g.,
convert the returned strings with Path(output["csv_file_path"]) and
Path(output["import_instructions"]) and either join their .name with data_dir
(data_dir / path.name) or resolve absolute paths (path.resolve()) before calling
.exists(); update the assertions that reference data_dir and the output dict to
use these Path operations so the test no longer depends on the exact "data/"
prefix format.
- Around line 82-116: Replace the manual try/finally environment save/restore
using previous_storage_root/previous_data_dir with pytest's monkeypatch fixture:
call monkeypatch.setenv("HIVE_AGENT_STORAGE_ROOT", tmpdir) and
monkeypatch.setenv("PROCUREMENT_APPROVAL_AGENT_DATA_DIR", str(data_dir)) before
invoking _run_workflow(mock_qb=True, ...), remove the try/finally block, and do
not manually restore those env vars; keep the assertions (qb_mock_path,
qb_records, etc.) and retain _set_qb_creds(enabled=False) in test teardown if
needed (or call it directly after assertions) so behavior of _run_workflow and
_set_qb_creds remains unchanged.
🪄 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: dec30202-35ab-4fb9-9c7d-07e7ef74990f
📒 Files selected for processing (5)
examples/templates/procurement_approval_agent/agent.pyexamples/templates/procurement_approval_agent/credentials.pyexamples/templates/procurement_approval_agent/nodes/quickbooks.pyexamples/templates/procurement_approval_agent/quickbooks_api.pyexamples/templates/procurement_approval_agent/tests/test_basic_workflow.py
✅ Files skipped from review due to trivial changes (1)
- examples/templates/procurement_approval_agent/credentials.py
🚧 Files skipped from review as they are similar to previous changes (2)
- examples/templates/procurement_approval_agent/nodes/quickbooks.py
- examples/templates/procurement_approval_agent/quickbooks_api.py
examples/templates/procurement_approval_agent/tests/test_basic_workflow.py
Show resolved
Hide resolved
examples/templates/procurement_approval_agent/tests/test_basic_workflow.py
Show resolved
Hide resolved
There was a problem hiding this comment.
♻️ Duplicate comments (1)
examples/templates/procurement_approval_agent/tests/test_basic_workflow.py (1)
50-59:⚠️ Potential issue | 🟠 MajorPreserve and restore pre-existing
QUICKBOOKS_*env values.
_set_qb_credsoverwrites process env, but cleanup paths only unset values. In particular, credentials are enabled at Line 266 and never restored in thefinallyblock (Lines 293-296). This can leak state to later tests and erase pre-set env values in the same pytest process.🔧 Suggested fix (snapshot + restore)
+QB_ENV_KEYS = ( + "QUICKBOOKS_CLIENT_ID", + "QUICKBOOKS_CLIENT_SECRET", + "QUICKBOOKS_REALM_ID", +) + -def _set_qb_creds(enabled: bool) -> None: +def _set_qb_creds(enabled: bool) -> dict[str, str | None]: + previous = {k: os.environ.get(k) for k in QB_ENV_KEYS} if enabled: os.environ["QUICKBOOKS_CLIENT_ID"] = "mock-client-id" os.environ["QUICKBOOKS_CLIENT_SECRET"] = "mock-client-secret" os.environ["QUICKBOOKS_REALM_ID"] = "mock-realm-id" else: os.environ.pop("QUICKBOOKS_CLIENT_ID", None) os.environ.pop("QUICKBOOKS_CLIENT_SECRET", None) os.environ.pop("QUICKBOOKS_REALM_ID", None) + return previous + + +def _restore_qb_creds(previous: dict[str, str | None]) -> None: + for key, value in previous.items(): + if value is None: + os.environ.pop(key, None) + else: + os.environ[key] = value- _set_qb_creds(enabled=True) + previous_qb_env = _set_qb_creds(enabled=True) ... finally: if previous_storage_root is None: os.environ.pop("HIVE_AGENT_STORAGE_ROOT", None) else: os.environ["HIVE_AGENT_STORAGE_ROOT"] = previous_storage_root + _restore_qb_creds(previous_qb_env)Also applies to: 264-296
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/templates/procurement_approval_agent/tests/test_basic_workflow.py` around lines 50 - 59, The helper _set_qb_creds currently overwrites QUICKBOOKS_* env vars without preserving prior values; modify it to save the original values (e.g., capture os.environ.get for QUICKBOOKS_CLIENT_ID, QUICKBOOKS_CLIENT_SECRET, QUICKBOOKS_REALM_ID) when setting enabled=True and return that snapshot so callers (tests using _set_qb_creds) can restore originals in the finally block; when called with enabled=False use the snapshot to restore previous values instead of blindly popping keys, and update the test that enables creds (the caller around Lines ~266-296) to capture the returned snapshot and restore it in its finally cleanup.
🧹 Nitpick comments (1)
examples/templates/procurement_approval_agent/tests/test_basic_workflow.py (1)
117-118: Remove duplicated teardown call on Line 118.
_set_qb_creds(enabled=False)is called twice consecutively; the second call is redundant and adds noise to cleanup logic.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/templates/procurement_approval_agent/tests/test_basic_workflow.py` around lines 117 - 118, The duplicate teardown call to _set_qb_creds(enabled=False) should be removed; keep a single invocation of _set_qb_creds(enabled=False) in the test cleanup to avoid redundant calls and noisy teardown logic—locate the consecutive calls to _set_qb_creds in the test_basic_workflow test and delete the second one so only one cleanup call remains.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@examples/templates/procurement_approval_agent/tests/test_basic_workflow.py`:
- Around line 50-59: The helper _set_qb_creds currently overwrites QUICKBOOKS_*
env vars without preserving prior values; modify it to save the original values
(e.g., capture os.environ.get for QUICKBOOKS_CLIENT_ID,
QUICKBOOKS_CLIENT_SECRET, QUICKBOOKS_REALM_ID) when setting enabled=True and
return that snapshot so callers (tests using _set_qb_creds) can restore
originals in the finally block; when called with enabled=False use the snapshot
to restore previous values instead of blindly popping keys, and update the test
that enables creds (the caller around Lines ~266-296) to capture the returned
snapshot and restore it in its finally cleanup.
---
Nitpick comments:
In `@examples/templates/procurement_approval_agent/tests/test_basic_workflow.py`:
- Around line 117-118: The duplicate teardown call to
_set_qb_creds(enabled=False) should be removed; keep a single invocation of
_set_qb_creds(enabled=False) in the test cleanup to avoid redundant calls and
noisy teardown logic—locate the consecutive calls to _set_qb_creds in the
test_basic_workflow test and delete the second one so only one cleanup call
remains.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 11db1096-99f4-4c93-bb59-dd1c3d84f567
📒 Files selected for processing (1)
examples/templates/procurement_approval_agent/tests/test_basic_workflow.py
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.
Description
Adds a Hive v0.6-compatible Procurement Approval Agent template with adaptive routing and continuous processing support, while preserving the expected interactive approval workflow.
Type of Change
Related Issues
Fixes #4987
Changes Made
examples/templates/procurement_approval_agent/including graph, nodes, config, CLI, monitoring, deployment helpers, and tests.uv runusage.Testing
Describe the tests you ran to verify your changes:
Additional verification run for this PR:
PYTHONPATH=examples/templates uv run pytest examples/templates/procurement_approval_agent/tests -q(12 passed)uv run python -m examples.templates.procurement_approval_agent validateuv run python -m examples.templates.procurement_approval_agent info --jsonChecklist
Demo Video
Demo recording: https://www.youtube.com/watch?v=wh2fun0sT48
Summary by CodeRabbit
New Features
Documentation
Tests