Add scoped agent research, memory, and RBAC - #396
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR adds role-ID Discord authorization, bounded public-web and ERP/Billing reads, tenant-scoped durable memory, recurring agent schedules, Discord diagnostics, worker dispatch, and dashboard management surfaces with associated migrations, tests, configuration, and documentation. ChangesAgent authorization and safety
Durable schedules and operational surfaces
Runtime delivery and validation
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_3d03ab4a-354b-4e19-bcf9-27ef840415df) |
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_4aafade4-4897-41d9-97c1-0a117182683b) |
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_1958e8cd-7792-43a0-8528-468812d96e23) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (13)
tests/unit/test_agent_postgres_memory.py (2)
211-241: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTest PAN is intentional; the scanner hit is a false positive.
4111 1111 1111 1111is the canonical non-issued test number used to exercise the Luhn detector. No change needed, though a# nosemgrep/inline suppression would keep the PII scan clean in CI.🤖 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 `@tests/unit/test_agent_postgres_memory.py` around lines 211 - 241, Suppress the intentional PII scanner finding on the PAN test value in test_remember_fact_rejects_sensitive_value_before_database_write, using the repository’s supported inline suppression syntax (such as nosemgrep) without changing the test data or behavior.Source: Linters/SAST tools
334-349: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider covering the
expected_organization_idpostcondition failure.
_memory_fact_from_rowraisesRuntimeError("Memory fact row violated its organization boundary")onremember_fact/forget_fact, but no test drives that path. A fake returning a row for the wrong tenant would lock in that last-line tenant guard.Also applies to: 369-390
🤖 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 `@tests/unit/test_agent_postgres_memory.py` around lines 334 - 349, The tests cover query filtering but not the tenant-boundary postcondition in _memory_fact_from_row. Add coverage for remember_fact or forget_fact using a fake cursor row whose organization_id differs from the expected organization, and assert that RuntimeError with “Memory fact row violated its organization boundary” is raised.tests/unit/test_agent_memory.py (2)
96-131: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd negative cases so the detector's false-positive surface is pinned down.
Every parametrized case asserts rejection; nothing asserts that benign text is accepted. Given how loose
_INLINE_SECRET_REis (seepackages/shared/src/five08/agent/memory.pyLine 38), a companion parametrized test over phrases like"password reset instructions","token bucket limit", and"the secret sauce doc"would make the regex tightening safe to land and prevent regressions.🤖 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 `@tests/unit/test_agent_memory.py` around lines 96 - 131, Add a companion parametrized test near test_in_memory_rejects_sensitive_values_before_storing covering benign phrases such as “password reset instructions,” “token bucket limit,” and “the secret sauce doc.” Assert these values are accepted and can be stored by InMemoryMemoryStore, preserving the existing rejection test for genuinely sensitive values.
173-193: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAssert through the public API rather than
store._facts.Both tests reach into the private dict.
purge_expiredalready returns a count andlist_factsexposes remaining rows per tenant, so the same assertions can be made without coupling to internals.Also applies to: 196-211
🤖 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 `@tests/unit/test_agent_memory.py` around lines 173 - 193, Update the tests around test_in_memory_purge_is_tenant_scoped_and_removes_expired_or_deleted_rows and the additionally referenced test to stop asserting against the private store._facts dictionary. Use purge_expired’s returned count and list_facts with the appropriate organization_id to verify remaining rows through the public API, preserving the tenant-scoped expectations.packages/shared/src/five08/agent/context.py (1)
91-99: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDerive the hard character cap from the model instead of hardcoding
2048.
AgentContextSnippet.textalready declaresmax_length=2048(packages/shared/src/five08/agent/models.pyLine 117). Duplicating the literal here means a model change silently desyncs the bounding logic. Also,estimate_context_tokensreturnsmax(1, ...), so thetoken_count <= 0branch is unreachable.♻️ Proposed refactor
- truncated_text = snippet.text[:2048] + truncated_text = snippet.text[:MAX_CONTEXT_SNIPPET_CHARS]Define the constant once (in
models.py) and reuse it for theField(max_length=...)declaration.🤖 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 `@packages/shared/src/five08/agent/context.py` around lines 91 - 99, Define a shared maximum-length constant in the module containing AgentContextSnippet, use it for the model field’s max_length, and import/reuse it when truncating snippet.text in the context-building logic instead of hardcoding 2048. Remove the unreachable token_count <= 0 check, while preserving the existing remaining-token filtering.packages/shared/src/five08/agent/memory.py (1)
274-283: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low value
_purge_expired_lockedruns a full-store scan on every read and write.
remember_fact,list_facts, andforget_facteach walk all facts across all tenants under the lock. At MVP scale this is fine, but it makes every memory operation O(total facts) and serializes them. If this store is used beyond tests, consider purging opportunistically (time-gated) rather than on every call.🤖 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 `@packages/shared/src/five08/agent/memory.py` around lines 274 - 283, The _purge_expired_locked flow currently scans the entire store for every memory operation. Add a time-gated purge policy so remember_fact, list_facts, and forget_fact reuse the existing purge logic only when the configured interval has elapsed, while preserving deletion of expired and deleted facts when purging runs and keeping access under the lock.tests/unit/test_diagnostics_cog.py (1)
145-157: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueFallback test only exercises the generic
except Exceptionbranch.
RuntimeErrorskips thediscord.Forbiddenanddiscord.HTTPExceptionhandlers, which produce a differentrefresh_errormessage. Adding adiscord.Forbiddencase would pin the user-visible copy for the most likely real failure (missing permissions).🤖 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 `@tests/unit/test_diagnostics_cog.py` around lines 145 - 157, Add a dedicated fallback test alongside test_cog_refresh_falls_back_to_gateway_cache using discord.Forbidden from guild.fetch_roles, then assert the 200 response, gateway_cache source, and the permission-specific refresh_error text. Keep the existing RuntimeError test to cover the generic exception branch.apps/api/src/five08/backend/api.py (1)
339-346: 🩺 Stability & Availability | 🔵 TrivialBulkhead bounds agent planner threads, but the shared default executor is still the real limit.
asyncio.to_threaduses the loop's defaultThreadPoolExecutor, which otherto_threadcallers in this module share. Timed-out planner threads hold both a bulkhead permit and an executor thread until the sync call returns, so under sustained timeouts unrelatedto_threadwork can still starve. Consider a dedicated executor for agent planning (or a metric on_AGENT_REQUEST_PLAN_BULKHEADsaturation) so the isolation is real and observable.Also applies to: 378-392
🤖 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/api/src/five08/backend/api.py` around lines 339 - 346, Update the agent planning flow that uses asyncio.to_thread to run synchronous planner work on a dedicated executor, isolating it from the event loop’s shared default executor while preserving the _AGENT_REQUEST_PLAN_BULKHEAD limit. Ensure the executor lifecycle is managed by the surrounding API application, and expose saturation through the existing AgentRequestPlanCapacityError path or an appropriate metric.apps/admin_dashboard/src/discord-diagnostics-view.test.tsx (1)
78-98: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider covering the clipboard and empty-state branches.
copyToClipboardhas three outcomes (unsupported clipboard, success, failure) that all driveonNotice, and thediagnostics === nullempty state has its own render path; neither is exercised here.🤖 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/admin_dashboard/src/discord-diagnostics-view.test.tsx` around lines 78 - 98, Extend the DiscordDiagnosticsView tests to cover all copyToClipboard outcomes—unsupported clipboard, successful copy, and rejected copy—and assert each corresponding onNotice result. Add a separate test rendering diagnostics={null} and verify the empty-state UI, while preserving the existing role filtering assertions.packages/shared/src/five08/agent/orchestrator.py (1)
411-441: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDrop the unused
_plan_with_modelparameter.
plan()is the only caller and it does not passexplicit_public_web_action, while the public-web branch returns before that call. This leaves the check in this block and the pass-through to_response_for_planner_resultunreachable; remove them or wire the flow explicitly.🤖 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 `@packages/shared/src/five08/agent/orchestrator.py` around lines 411 - 441, Update the planner flow around _plan_with_model and its sole caller plan(): remove the unused explicit_public_web_action parameter and eliminate the unreachable public-web validation/pass-through logic in that block. Preserve the existing deterministic explicit-public-web handling and ensure _response_for_planner_result receives only values that remain reachable and necessary.packages/shared/src/five08/settings.py (2)
93-107: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider enforcing
agent_public_web_deadline_seconds < agent_request_response_budget_seconds.Both bounds are independently configurable (e.g. budget
10, deadline50), so a misconfiguration silently converts public-web research into 504s. The docs state the invariant; the existingmodel_validatorcould assert it.♻️ Sketch
`@model_validator`(mode="after") def _reject_everyone_agent_role_bindings(self) -> "SharedSettings": + # (or a separate validator) + if self.agent_public_web_deadline_seconds >= ( + self.agent_request_response_budget_seconds + ): + raise ValueError( + "AGENT_PUBLIC_WEB_DEADLINE_SECONDS must stay below " + "AGENT_REQUEST_RESPONSE_BUDGET_SECONDS" + )🤖 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 `@packages/shared/src/five08/settings.py` around lines 93 - 107, Update the settings model’s existing model_validator to enforce that agent_public_web_deadline_seconds is strictly less than agent_request_response_budget_seconds, rejecting configurations such as budget 10 with deadline 50 while preserving valid configurations and existing field bounds.
422-430: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winShare the local-environment set with
PolicyEngine.from_settings.The same
{"local", "development", "dev", "test", "testing"}literal is duplicated inpackages/shared/src/five08/agent/policy.py(line 216) where it decidesrequire_guild_binding. Divergence between the two copies would silently change the fail-closed boundary; export one constant fromsettings.pyand reuse it.🤖 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 `@packages/shared/src/five08/settings.py` around lines 422 - 430, Define and export a single shared local-environment set in settings.py, then update agent_role_name_fallback_enabled and PolicyEngine.from_settings to reuse it instead of maintaining duplicate literals. Preserve the existing normalized environment matching and require_guild_binding behavior.tests/unit/test_agent_web.py (1)
345-406: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider covering the
deadline_monotonicpath.The suite exercises provider fallback and typed errors thoroughly, but nothing asserts
_effective_request_timeoutbehavior: that a supplied deadline clamps the per-request timeout, and that an already-expired deadline raisesWebResearchTransportErrorbefore any HTTP call. That is the control bounding agent latency for public web reads.🤖 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 `@tests/unit/test_agent_web.py` around lines 345 - 406, Add unit tests covering WebResearchClient’s deadline_monotonic path: verify a supplied deadline clamps the effective per-request timeout, and verify an already-expired deadline raises WebResearchTransportError before any provider or HTTP call occurs. Reuse the existing provider test patterns and assert the transport error and zero provider-call behavior.
🤖 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 `@packages/shared/src/five08/agent/context.py`:
- Around line 17-24: Update _contains_private_context_text and its
snippet-processing flow to avoid dropping an entire request-context snippet
merely because snippet.text contains a UUID; narrowly classify only genuinely
sensitive UUIDs or redact matched tokens while preserving surrounding context.
Add a production counter or log for snippets that are dropped or become blank,
using the existing context-handling symbols and observability mechanisms.
In `@packages/shared/src/five08/agent/memory.py`:
- Around line 38-47: N/A
- Line 64: Replace the shape-only pattern in _IBAN_RE with IBAN validation that
accepts lowercase input case-insensitively, restricts matches to a real IBAN
country-code set, and verifies the rearranged IBAN mod-97 checksum. Preserve
matching only valid IBANs so unrelated uppercase identifiers such as
SKU12ABCDEFGHIJKL are rejected.
In `@packages/shared/src/five08/agent/postgres_memory.py`:
- Around line 19-26: Make the shared helper contract explicit by renaming
_assert_visible_org_matches_tenant and _normalized_time in five08.agent.memory
to public names, then update postgres_memory.py and all other call sites to use
those names consistently. Alternatively, move both helpers into a shared
_memory_common module and import them from there, removing cross-module imports
of the underscore-prefixed symbols.
- Around line 231-232: Remove the _purge_expired_with_cursor call from the
list_facts read path so reads perform no deletion or locking; rely on the
existing expires_at > %s predicate to hide expired rows. Move expiry cleanup to
a scheduled worker purge_expired invocation, and update include_deleted handling
consistently—either preserve its soft-delete filtering for rows that can exist
or remove the unused parameter and related conditions, noting that forget_fact
performs physical deletion.
- Around line 47-63: Update PostgresMemoryStore.__init__ and its database-access
path to enforce bounded psycopg connection timeouts and a driver-supported
statement timeout for every operation. Reuse a connection pool for normal agent
traffic so memory operations do not create a fresh TCP/TLS/auth connection each
time, while preserving the injectable ConnectionFactory behavior for tests and
callers that provide one.
---
Nitpick comments:
In `@apps/admin_dashboard/src/discord-diagnostics-view.test.tsx`:
- Around line 78-98: Extend the DiscordDiagnosticsView tests to cover all
copyToClipboard outcomes—unsupported clipboard, successful copy, and rejected
copy—and assert each corresponding onNotice result. Add a separate test
rendering diagnostics={null} and verify the empty-state UI, while preserving the
existing role filtering assertions.
In `@apps/api/src/five08/backend/api.py`:
- Around line 339-346: Update the agent planning flow that uses
asyncio.to_thread to run synchronous planner work on a dedicated executor,
isolating it from the event loop’s shared default executor while preserving the
_AGENT_REQUEST_PLAN_BULKHEAD limit. Ensure the executor lifecycle is managed by
the surrounding API application, and expose saturation through the existing
AgentRequestPlanCapacityError path or an appropriate metric.
In `@packages/shared/src/five08/agent/context.py`:
- Around line 91-99: Define a shared maximum-length constant in the module
containing AgentContextSnippet, use it for the model field’s max_length, and
import/reuse it when truncating snippet.text in the context-building logic
instead of hardcoding 2048. Remove the unreachable token_count <= 0 check, while
preserving the existing remaining-token filtering.
In `@packages/shared/src/five08/agent/memory.py`:
- Around line 274-283: The _purge_expired_locked flow currently scans the entire
store for every memory operation. Add a time-gated purge policy so
remember_fact, list_facts, and forget_fact reuse the existing purge logic only
when the configured interval has elapsed, while preserving deletion of expired
and deleted facts when purging runs and keeping access under the lock.
In `@packages/shared/src/five08/agent/orchestrator.py`:
- Around line 411-441: Update the planner flow around _plan_with_model and its
sole caller plan(): remove the unused explicit_public_web_action parameter and
eliminate the unreachable public-web validation/pass-through logic in that
block. Preserve the existing deterministic explicit-public-web handling and
ensure _response_for_planner_result receives only values that remain reachable
and necessary.
In `@packages/shared/src/five08/settings.py`:
- Around line 93-107: Update the settings model’s existing model_validator to
enforce that agent_public_web_deadline_seconds is strictly less than
agent_request_response_budget_seconds, rejecting configurations such as budget
10 with deadline 50 while preserving valid configurations and existing field
bounds.
- Around line 422-430: Define and export a single shared local-environment set
in settings.py, then update agent_role_name_fallback_enabled and
PolicyEngine.from_settings to reuse it instead of maintaining duplicate
literals. Preserve the existing normalized environment matching and
require_guild_binding behavior.
In `@tests/unit/test_agent_memory.py`:
- Around line 96-131: Add a companion parametrized test near
test_in_memory_rejects_sensitive_values_before_storing covering benign phrases
such as “password reset instructions,” “token bucket limit,” and “the secret
sauce doc.” Assert these values are accepted and can be stored by
InMemoryMemoryStore, preserving the existing rejection test for genuinely
sensitive values.
- Around line 173-193: Update the tests around
test_in_memory_purge_is_tenant_scoped_and_removes_expired_or_deleted_rows and
the additionally referenced test to stop asserting against the private
store._facts dictionary. Use purge_expired’s returned count and list_facts with
the appropriate organization_id to verify remaining rows through the public API,
preserving the tenant-scoped expectations.
In `@tests/unit/test_agent_postgres_memory.py`:
- Around line 211-241: Suppress the intentional PII scanner finding on the PAN
test value in test_remember_fact_rejects_sensitive_value_before_database_write,
using the repository’s supported inline suppression syntax (such as nosemgrep)
without changing the test data or behavior.
- Around line 334-349: The tests cover query filtering but not the
tenant-boundary postcondition in _memory_fact_from_row. Add coverage for
remember_fact or forget_fact using a fake cursor row whose organization_id
differs from the expected organization, and assert that RuntimeError with
“Memory fact row violated its organization boundary” is raised.
In `@tests/unit/test_agent_web.py`:
- Around line 345-406: Add unit tests covering WebResearchClient’s
deadline_monotonic path: verify a supplied deadline clamps the effective
per-request timeout, and verify an already-expired deadline raises
WebResearchTransportError before any provider or HTTP call occurs. Reuse the
existing provider test patterns and assert the transport error and zero
provider-call behavior.
In `@tests/unit/test_diagnostics_cog.py`:
- Around line 145-157: Add a dedicated fallback test alongside
test_cog_refresh_falls_back_to_gateway_cache using discord.Forbidden from
guild.fetch_roles, then assert the 200 response, gateway_cache source, and the
permission-specific refresh_error text. Keep the existing RuntimeError test to
cover the generic exception branch.
🪄 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 Plus
Run ID: 430dd4e2-8c33-4d2a-a15d-9faa92cea943
📒 Files selected for processing (57)
.env.exampleapps/admin_dashboard/src/discord-diagnostics-view.test.tsxapps/admin_dashboard/src/main.tsxapps/admin_dashboard/src/views/discord-diagnostics-view.tsxapps/api/README.mdapps/api/src/five08/backend/api.pyapps/api/src/five08/backend/routes.pyapps/api/src/five08/backend/static/dashboard/.vite/manifest.jsonapps/api/src/five08/backend/static/dashboard/assets/index-2UypYUrJ.cssapps/api/src/five08/backend/static/dashboard/assets/index-CjyViga_.jsapps/api/src/five08/backend/static/dashboard/assets/index-DVOtKdpK.jsapps/api/src/five08/backend/static/dashboard/assets/index-Dkp5BMUm.cssapps/api/src/five08/backend/static/dashboard/index.htmlapps/discord_bot/README.mdapps/discord_bot/src/five08/discord_bot/cogs/agent.pyapps/discord_bot/src/five08/discord_bot/cogs/diagnostics.pyapps/discord_bot/src/five08/discord_bot/config.pyapps/discord_bot/src/five08/discord_bot/utils/internal_api.pyapps/worker/src/five08/worker/config.pyapps/worker/src/five08/worker/migrations/versions/20260728_0100_create_agent_memory_facts.pycompose.yamldocs/configuration.mdpackages/shared/src/five08/agent/__init__.pypackages/shared/src/five08/agent/context.pypackages/shared/src/five08/agent/evals.pypackages/shared/src/five08/agent/memory.pypackages/shared/src/five08/agent/models.pypackages/shared/src/five08/agent/orchestrator.pypackages/shared/src/five08/agent/planner.pypackages/shared/src/five08/agent/policy.pypackages/shared/src/five08/agent/postgres_memory.pypackages/shared/src/five08/agent/tools.pypackages/shared/src/five08/agent/web.pypackages/shared/src/five08/settings.pytests/evals/discord-agent/fixtures/v1/context_prompt_injection_ignored_001.jsontests/evals/discord-agent/fixtures/v1/create_task_confirmation_001.jsontests/evals/discord-agent/fixtures/v1/github_issue_member_denied_001.jsontests/evals/discord-agent/fixtures/v1/memory_read_self_001.jsontests/evals/discord-agent/fixtures/v1/memory_remember_confirmation_001.jsontests/evals/discord-agent/fixtures/v1/missing_project_clarification_001.jsontests/evals/discord-agent/fixtures/v1/search_project_tasks_001.jsontests/evals/discord-agent/fixtures/v1/task_assign_member_denied_001.jsontests/evals/discord-agent/fixtures/v1/task_complete_confirmation_001.jsontests/evals/discord-agent/fixtures/v1/task_create_mentions_github_issue_001.jsontests/evals/discord-agent/fixtures/v1/task_project_prefixed_search_001.jsontests/evals/discord-agent/fixtures/v1/thread_followup_latest_message_001.jsontests/unit/test_agent_cog.pytests/unit/test_agent_erp_tools.pytests/unit/test_agent_gateway.pytests/unit/test_agent_memory.pytests/unit/test_agent_postgres_memory.pytests/unit/test_agent_role_bindings.pytests/unit/test_agent_web.pytests/unit/test_backend_api.pytests/unit/test_diagnostics_cog.pytests/unit/test_internal_api.pytests/unit/test_shared_settings.py
💤 Files with no reviewable changes (2)
- apps/api/src/five08/backend/static/dashboard/assets/index-2UypYUrJ.css
- apps/worker/src/five08/worker/config.py
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_2bd98a6f-056d-4975-8a27-368fd43a25db) |
![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: 8375bcab77
ℹ️ 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".
| orchestrator.registry.validate_planner_action( | ||
| configured_action.tool_name, | ||
| configured_action.arguments, | ||
| ) |
There was a problem hiding this comment.
When an admin supplies a syntactically valid repository that is absent from GITHUB_DEFAULT_REPO/GITHUB_ALLOWED_REPOS, this validation succeeds because validate_planner_action checks only argument shape. The schedule is persisted as active, but every occurrence later fails in ToolRegistry._resolve_repository, producing repeated failed worker runs instead of rejecting the unusable schedule during creation. Validate the frozen repository against the configured allowlist here before persisting it.
Useful? React with 👍 / 👎.
| prompt: str = Field(min_length=1, max_length=4_000) | ||
| repository: str = Field(min_length=3, max_length=256) | ||
| query: str = Field(default="", max_length=512) | ||
| state: Literal["open", "closed", "all"] = "open" |
There was a problem hiding this comment.
When an API caller creates a schedule with state: "all", that value is frozen and later passed to GitHubClient.search_issues, which emits the query qualifier state:all. GitHub issue search uses state:open or state:closed; searching both states requires omitting the qualifier, so the advertised all option produces an incorrect or empty recurring report. Normalize all to no state qualifier, or remove it from the accepted schema.
Useful? React with 👍 / 👎.
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_fdf16fc5-a727-49bc-9554-809cd0eee2b5) |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (11)
apps/worker/src/five08/worker/migrations/versions/20260728_0200_create_agent_schedules.py (1)
165-197: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low value
idx_agent_schedule_runs_schedule_occurrenceduplicates the unique constraint's index.
uq_agent_schedule_runs_schedule_occurrencealready creates a btree on(schedule_id, occurrence_at), which serveslist_agent_schedule_runs. Dropping the extra index removes write amplification with no read cost.🤖 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/worker/src/five08/worker/migrations/versions/20260728_0200_create_agent_schedules.py` around lines 165 - 197, Remove the redundant idx_agent_schedule_runs_schedule_occurrence index creation from the migration, while retaining the uq_agent_schedule_runs_schedule_occurrence unique constraint to provide the required (schedule_id, occurrence_at) index.tests/unit/test_worker_agent_schedules.py (1)
13-59: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider covering the retryable branch too.
Only the 403 non-retryable path is asserted. A 500 response should raise plain
RuntimeError(retryable); pinning that distinction protects the retry policy from an accidental widening of the non-retryable status set.🤖 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 `@tests/unit/test_worker_agent_schedules.py` around lines 13 - 59, The tests for run_agent_schedule_job cover only the 403 non-retryable response; add a test for a 500 response that asserts plain RuntimeError is raised, preserving the distinction between retryable server errors and AgentScheduleRunNonRetryableError policy rejections.packages/shared/src/five08/agent/web.py (1)
698-761: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winStreamed reads can outlive the per-call deadline.
timeoutonrequestsis a per-read (socket) timeout, not a wall-clock budget. Withstream=True, a drip-feeding upstream can keep the body read alive for up toceil(MAX_WEB_RESPONSE_BYTES / WEB_RESPONSE_CHUNK_BYTES)× timeout, bypassing the monotonic deadline that_effective_request_timeoutcomputes. Consider re-checking the deadline inside theiter_contentloop and raisingWebResearchTransportErrorwhen it has passed.🤖 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 `@packages/shared/src/five08/agent/web.py` around lines 698 - 761, Update _read_bounded_json_response to enforce the wall-clock deadline during streamed reads by re-checking the request deadline inside the response.iter_content loop. When the deadline has passed, raise WebResearchTransportError and preserve the existing response-closing behavior in _response_json; reuse the deadline computed by _effective_request_timeout rather than introducing a separate timeout budget.apps/admin_dashboard/src/agent-schedules-view.test.tsx (1)
32-57: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a
canWrite={false}case.The lifecycle controls are permission-gated, but only the fully-privileged render is asserted. A case verifying Pause/Run are absent (or disabled) for read-only viewers would guard the RBAC surface this PR is built around.
🤖 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/admin_dashboard/src/agent-schedules-view.test.tsx` around lines 32 - 57, Add a separate read-only test for AgentSchedulesView with canWrite={false}, while keeping the existing schedule setup and lifecycle callbacks. Assert that the Pause and Run controls are absent or disabled, covering the permission-gated RBAC behavior without changing the privileged test.apps/discord_bot/src/five08/discord_bot/cogs/schedules.py (2)
230-254: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRole-snapshot normalization is duplicated in
apps/discord_bot/src/five08/discord_bot/utils/internal_api.py(Lines 604-612).Same decimal/dedupe loop for
role_ids/rolesexists in both places. Consider extracting a shared helper in the bot package so the two stay in sync.🤖 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/discord_bot/src/five08/discord_bot/cogs/schedules.py` around lines 230 - 254, Extract the duplicated role snapshot normalization loop from `_context` and `internal_api.py` into a shared helper in the bot package. Update both call sites to use it, preserving positive decimal role ID filtering, deduplication, and role-name collection behavior.
174-177: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winPercent-encode
schedule_idbefore interpolating it into the backend path.
schedule_idcomes straight from user slash-command input. Characters like/,.., or whitespace are pasted unescaped into the request URL, letting a caller reshape which backend path is hit. Wrap it withurllib.parse.quote(schedule_id, safe="").🔒 Proposed fix
+from urllib.parse import quote @@ response = await self._post_backend( - f"/agent/schedules/{schedule_id}/control", + f"/agent/schedules/{quote(schedule_id, safe='')}/control", {"context": context, "action": action.value}, ) @@ response = await self._post_backend( - f"/agent/schedules/{schedule_id}/run", + f"/agent/schedules/{quote(schedule_id, safe='')}/run", {"context": context}, )Also applies to: 213-216
🤖 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/discord_bot/src/five08/discord_bot/cogs/schedules.py` around lines 174 - 177, Update the schedule control request in the relevant handler to percent-encode the user-provided schedule_id with urllib.parse.quote(schedule_id, safe="") before interpolating it into the /agent/schedules/{schedule_id}/control path, and apply the same change to the additional request path noted in the comment.tests/unit/test_backend_agent_schedules.py (1)
197-258: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest name promises "only" but never exercises the non-stale case.
Only the reclaimable path is asserted. Add a companion case where
started_atis recent and assert_execute_agent_schedule_runreturns 409schedule_run_already_runningwithclaim_agent_schedule_runnot called — that's the boundary that actually protects against double execution.🤖 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 `@tests/unit/test_backend_agent_schedules.py` around lines 197 - 258, Add a companion test for _execute_agent_schedule_run using a RUNNING run with a recent started_at value; assert it returns HTTP 409 with “schedule_run_already_running” and verify claim_agent_schedule_run is not called. Keep the existing stale-run reclaim assertions unchanged.apps/admin_dashboard/src/views/agent-schedules-view.tsx (1)
186-304: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider wrapping the create fields in a
<form>withonSubmit.Currently there's no form element, so Enter in any input does nothing and screen-reader users get no form semantics. A
<form onSubmit={(e) => { e.preventDefault(); void submit() }}>with atype="submit"button keeps the same behavior and restores keyboard submission.🤖 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/admin_dashboard/src/views/agent-schedules-view.tsx` around lines 186 - 304, Wrap the schedule creation fields and create button in a form with an onSubmit handler that prevents the default action and invokes submit(). Change the Create schedule Button to type="submit", preserving the existing canSubmit disabled state and field behavior while enabling Enter-key submission and form semantics.apps/api/src/five08/backend/api.py (3)
10239-10248: 🚀 Performance & Scalability | 🔵 TrivialNote: a planner timeout does not free the thread or the bulkhead permit immediately.
asyncio.wait_forcancels the await, but theto_threadworker keeps running untilorchestrator.planreturns, so the permit is released only then. Under sustained overload, callers will see 503 (capacity) rather than 504 (timeout). That is the intended bulkhead behavior; just make sure_AGENT_REQUEST_PLAN_BULKHEAD's bound is not larger than the defaultasyncio.to_threadexecutor size, otherwise the executor saturates before the bulkhead rejects.🤖 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/api/src/five08/backend/api.py` around lines 10239 - 10248, Ensure the configured bound for _AGENT_REQUEST_PLAN_BULKHEAD does not exceed the default asyncio.to_thread executor capacity, so the bulkhead rejects excess work before the executor saturates. Preserve the existing asyncio.wait_for timeout and worker/permit behavior.
9648-9667: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winConsider auditing denied schedule-management attempts.
_agent_schedule_manager_contextreturns a 403 payload without emitting an audit event, so failed attempts to pause/resume/archive/run a schedule leave no trace, while successes do. A deniedagent.schedule.*audit event here would make privilege-revocation and probing visible.🤖 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/api/src/five08/backend/api.py` around lines 9648 - 9667, Update _agent_schedule_manager_context to emit an audit event whenever _agent_schedule_manager_error returns a denial, before returning the 403 payload. Record the denied agent.schedule.* management attempt with the relevant request/context details, while preserving the existing authorization response and success behavior.
7906-7928: 🚀 Performance & Scalability | 🔵 TrivialConsider a server-side cooldown for
?refresh=true.Any
configuration:readsession can repeatedly force a live Discord fetch through this proxy. If the diagnostics cog does not already throttle refreshes, a short cooldown (or cached-snapshot reuse) would protect the bot's Discord rate-limit budget.🤖 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/api/src/five08/backend/api.py` around lines 7906 - 7928, Update dashboard_discord_diagnostics_handler and the associated refresh path to enforce a short server-side cooldown for refresh=true, reusing the most recent diagnostics snapshot while the cooldown is active. Preserve normal cached reads, permission checks, no-store responses, and the existing 503 behavior when no snapshot is available.
🤖 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/api/src/five08/backend/api.py`:
- Around line 9527-9560: The agent schedule delivery flow around
_post_agent_schedule_report_to_bot must become idempotent for retries: establish
a durable pre-delivery marker or bot-side deduplication keyed by (schedule.id,
run.id) before channel.send, and have retries detect the existing delivery and
reuse it instead of posting again. Preserve the existing failure completion and
response handling for genuine delivery errors.
In `@packages/shared/src/five08/agent/schedules.py`:
- Around line 369-382: Normalize incoming schedule_id and run_id values as UUIDs
before executing database queries, treating parse failures as not found/no-op
results rather than allowing database errors. Apply this consistently in
get_agent_schedule, pause_agent_schedule, resume_agent_schedule,
archive_agent_schedule, create_manual_agent_schedule_run,
claim_agent_schedule_run, complete_agent_schedule_run,
set_agent_schedule_run_job_id, and list_agent_schedule_runs, while preserving
existing behavior for valid identifiers.
- Around line 287-291: Update the cadence validation error in the schedule
normalization flow to report the same effective minimum used by the check:
max(60, minimum_interval_seconds). Keep the enforcement condition and successful
return values unchanged.
---
Nitpick comments:
In `@apps/admin_dashboard/src/agent-schedules-view.test.tsx`:
- Around line 32-57: Add a separate read-only test for AgentSchedulesView with
canWrite={false}, while keeping the existing schedule setup and lifecycle
callbacks. Assert that the Pause and Run controls are absent or disabled,
covering the permission-gated RBAC behavior without changing the privileged
test.
In `@apps/admin_dashboard/src/views/agent-schedules-view.tsx`:
- Around line 186-304: Wrap the schedule creation fields and create button in a
form with an onSubmit handler that prevents the default action and invokes
submit(). Change the Create schedule Button to type="submit", preserving the
existing canSubmit disabled state and field behavior while enabling Enter-key
submission and form semantics.
In `@apps/api/src/five08/backend/api.py`:
- Around line 10239-10248: Ensure the configured bound for
_AGENT_REQUEST_PLAN_BULKHEAD does not exceed the default asyncio.to_thread
executor capacity, so the bulkhead rejects excess work before the executor
saturates. Preserve the existing asyncio.wait_for timeout and worker/permit
behavior.
- Around line 9648-9667: Update _agent_schedule_manager_context to emit an audit
event whenever _agent_schedule_manager_error returns a denial, before returning
the 403 payload. Record the denied agent.schedule.* management attempt with the
relevant request/context details, while preserving the existing authorization
response and success behavior.
- Around line 7906-7928: Update dashboard_discord_diagnostics_handler and the
associated refresh path to enforce a short server-side cooldown for
refresh=true, reusing the most recent diagnostics snapshot while the cooldown is
active. Preserve normal cached reads, permission checks, no-store responses, and
the existing 503 behavior when no snapshot is available.
In `@apps/discord_bot/src/five08/discord_bot/cogs/schedules.py`:
- Around line 230-254: Extract the duplicated role snapshot normalization loop
from `_context` and `internal_api.py` into a shared helper in the bot package.
Update both call sites to use it, preserving positive decimal role ID filtering,
deduplication, and role-name collection behavior.
- Around line 174-177: Update the schedule control request in the relevant
handler to percent-encode the user-provided schedule_id with
urllib.parse.quote(schedule_id, safe="") before interpolating it into the
/agent/schedules/{schedule_id}/control path, and apply the same change to the
additional request path noted in the comment.
In
`@apps/worker/src/five08/worker/migrations/versions/20260728_0200_create_agent_schedules.py`:
- Around line 165-197: Remove the redundant
idx_agent_schedule_runs_schedule_occurrence index creation from the migration,
while retaining the uq_agent_schedule_runs_schedule_occurrence unique constraint
to provide the required (schedule_id, occurrence_at) index.
In `@packages/shared/src/five08/agent/web.py`:
- Around line 698-761: Update _read_bounded_json_response to enforce the
wall-clock deadline during streamed reads by re-checking the request deadline
inside the response.iter_content loop. When the deadline has passed, raise
WebResearchTransportError and preserve the existing response-closing behavior in
_response_json; reuse the deadline computed by _effective_request_timeout rather
than introducing a separate timeout budget.
In `@tests/unit/test_backend_agent_schedules.py`:
- Around line 197-258: Add a companion test for _execute_agent_schedule_run
using a RUNNING run with a recent started_at value; assert it returns HTTP 409
with “schedule_run_already_running” and verify claim_agent_schedule_run is not
called. Keep the existing stale-run reclaim assertions unchanged.
In `@tests/unit/test_worker_agent_schedules.py`:
- Around line 13-59: The tests for run_agent_schedule_job cover only the 403
non-retryable response; add a test for a 500 response that asserts plain
RuntimeError is raised, preserving the distinction between retryable server
errors and AgentScheduleRunNonRetryableError policy rejections.
🪄 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 Plus
Run ID: 32e20c24-4c1b-4153-ae9b-1cd450da85a6
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (38)
.env.exampleapps/admin_dashboard/src/agent-schedules-view.test.tsxapps/admin_dashboard/src/main.tsxapps/admin_dashboard/src/views/agent-schedules-view.tsxapps/api/README.mdapps/api/src/five08/backend/api.pyapps/api/src/five08/backend/routes.pyapps/api/src/five08/backend/schemas.pyapps/api/src/five08/backend/static/dashboard/.vite/manifest.jsonapps/api/src/five08/backend/static/dashboard/assets/index-C2hkz1-S.cssapps/api/src/five08/backend/static/dashboard/assets/index-DvWm4SlD.jsapps/api/src/five08/backend/static/dashboard/index.htmlapps/discord_bot/README.mdapps/discord_bot/src/five08/discord_bot/cogs/schedules.pyapps/discord_bot/src/five08/discord_bot/utils/internal_api.pyapps/worker/src/five08/worker/actors.pyapps/worker/src/five08/worker/config.pyapps/worker/src/five08/worker/jobs.pyapps/worker/src/five08/worker/migrations/versions/20260728_0200_create_agent_schedules.pycompose.yamldocs/configuration.mdpackages/shared/pyproject.tomlpackages/shared/src/five08/agent/__init__.pypackages/shared/src/five08/agent/policy.pypackages/shared/src/five08/agent/postgres_memory.pypackages/shared/src/five08/agent/schedules.pypackages/shared/src/five08/agent/tools.pypackages/shared/src/five08/agent/web.pypackages/shared/src/five08/settings.pytests/unit/test_agent_postgres_memory.pytests/unit/test_agent_role_bindings.pytests/unit/test_agent_schedules.pytests/unit/test_agent_web.pytests/unit/test_backend_agent_schedules.pytests/unit/test_backend_api.pytests/unit/test_internal_api.pytests/unit/test_shared_settings.pytests/unit/test_worker_agent_schedules.py
🚧 Files skipped from review as they are similar to previous changes (11)
- apps/api/src/five08/backend/static/dashboard/.vite/manifest.json
- tests/unit/test_shared_settings.py
- apps/discord_bot/README.md
- docs/configuration.md
- packages/shared/src/five08/agent/postgres_memory.py
- tests/unit/test_agent_role_bindings.py
- tests/unit/test_agent_postgres_memory.py
- packages/shared/src/five08/agent/policy.py
- packages/shared/src/five08/settings.py
- packages/shared/src/five08/agent/tools.py
- tests/unit/test_backend_api.py
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 91e99d2fbf
ℹ️ 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".
| return self._response_for_actions( | ||
| actions=next_actions, | ||
| context=context, | ||
| planning_text=planning_text, | ||
| planner="live_model", | ||
| model=result.model, |
There was a problem hiding this comment.
After a public search, the result snippets are untrusted web content, but the planner's next drafted action is passed directly back into _response_for_actions; the subsequent checks constrain tool shape and scope but do not establish that the user selected the new query or result. A prompt-injected search result can therefore steer an additional outbound search or extraction request. Select follow-up actions in code or require the user to choose the next result instead of using the LLM for this routing decision.
AGENTS.md reference: AGENTS.md:L130-L130
Useful? React with 👍 / 👎.
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_275d0a7c-ee00-4983-a527-8af4a4fa505c) |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/discord_bot/src/five08/discord_bot/cogs/schedules.py (1)
27-172: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider extracting shared boilerplate between
/schedule-github-issuesand/schedule-agent.Both commands repeat the same defer →
_contextguild check → channel-guild check → backend post → status/shape error handling sequence almost verbatim. With a second command now following this pattern, extracting a shared helper (e.g._guard_and_post(interaction, payload, path, success_message_fn)) would reduce duplication and keep future schedule commands consistent.🤖 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/discord_bot/src/five08/discord_bot/cogs/schedules.py` around lines 27 - 172, Extract the duplicated defer, context validation, channel-guild validation, backend posting, response-status handling, and schedule-shape validation from schedule_github_issues_command and schedule_agent_command into a shared helper such as _guard_and_post. Have each command provide its payload and success-message construction while preserving the existing user-facing messages and response behavior.
🤖 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.
Nitpick comments:
In `@apps/discord_bot/src/five08/discord_bot/cogs/schedules.py`:
- Around line 27-172: Extract the duplicated defer, context validation,
channel-guild validation, backend posting, response-status handling, and
schedule-shape validation from schedule_github_issues_command and
schedule_agent_command into a shared helper such as _guard_and_post. Have each
command provide its payload and success-message construction while preserving
the existing user-facing messages and response behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f349518c-13d4-4eec-8f8d-9823ae335975
📒 Files selected for processing (27)
.env.exampleapps/admin_dashboard/src/agent-schedules-view.test.tsxapps/admin_dashboard/src/views/agent-schedules-view.tsxapps/api/README.mdapps/api/src/five08/backend/api.pyapps/api/src/five08/backend/schemas.pyapps/api/src/five08/backend/static/dashboard/.vite/manifest.jsonapps/api/src/five08/backend/static/dashboard/assets/index-BZXfv05L.cssapps/api/src/five08/backend/static/dashboard/assets/index-Buji_yBE.jsapps/api/src/five08/backend/static/dashboard/index.htmlapps/discord_bot/README.mdapps/discord_bot/src/five08/discord_bot/cogs/schedules.pydocs/configuration.mdpackages/shared/src/five08/agent/__init__.pypackages/shared/src/five08/agent/context.pypackages/shared/src/five08/agent/memory.pypackages/shared/src/five08/agent/planner.pypackages/shared/src/five08/agent/postgres_memory.pypackages/shared/src/five08/agent/schedules.pypackages/shared/src/five08/agent/tools.pypackages/shared/src/five08/settings.pytests/unit/test_agent_erp_tools.pytests/unit/test_agent_gateway.pytests/unit/test_agent_memory.pytests/unit/test_agent_postgres_memory.pytests/unit/test_agent_schedules.pytests/unit/test_backend_agent_schedules.py
🚧 Files skipped from review as they are similar to previous changes (13)
- apps/api/src/five08/backend/static/dashboard/index.html
- apps/api/README.md
- apps/discord_bot/README.md
- tests/unit/test_agent_schedules.py
- .env.example
- packages/shared/src/five08/settings.py
- packages/shared/src/five08/agent/planner.py
- docs/configuration.md
- apps/api/src/five08/backend/schemas.py
- tests/unit/test_agent_postgres_memory.py
- tests/unit/test_agent_gateway.py
- packages/shared/src/five08/agent/memory.py
- packages/shared/src/five08/agent/tools.py
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6890a6f2aa
ℹ️ 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".
| normalized_timezone, | ||
| after=first, | ||
| ) | ||
| if (second - first).total_seconds() < max(60, minimum_interval_seconds): |
There was a problem hiding this comment.
When AGENT_SCHEDULE_MIN_INTERVAL_SECONDS exceeds one hour, checking only the first two occurrences can accept schedules that later violate the configured minimum. For example, with a 7,200-second minimum, creating 0 0,1 * * * at 00:30 compares 01:00 to the following midnight and passes, but that midnight run is followed by another at 01:00, only one hour later. Validate enough successive occurrences to cover the cron cycle so the configured cost/rate bound is actually enforced.
Useful? React with 👍 / 👎.
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_a480f08e-440d-4a1b-ae26-4374143220f3) |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/unit/test_agent_cog.py (1)
33-42: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAvoid enabling name-based authorization for every test in this module.
The autouse fixture enables the local role-name fallback globally, and the new admin test passes
role_ids=[]. This verifies"Admin"name fallback rather than the production role-ID plus guild-bound path. Scope the fallback to only legacy tests and add the admin assertion with a configured role ID; otherwise role-ID authorization regressions can remain green.Also applies to: 152-161
🤖 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 `@tests/unit/test_agent_cog.py` around lines 33 - 42, Remove the autouse behavior from _enable_explicit_local_role_name_fallback so name-based authorization is not enabled for every test in the module. Apply the fallback fixture only to legacy role-name tests, and update the admin authorization test to configure and assert against a non-empty role ID while preserving the guild-bound authorization path.
🤖 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.
Nitpick comments:
In `@tests/unit/test_agent_cog.py`:
- Around line 33-42: Remove the autouse behavior from
_enable_explicit_local_role_name_fallback so name-based authorization is not
enabled for every test in the module. Apply the fallback fixture only to legacy
role-name tests, and update the admin authorization test to configure and assert
against a non-empty role ID while preserving the guild-bound authorization path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 26d74eb5-e2a6-4f90-a58b-fa3d0be137f9
📒 Files selected for processing (14)
apps/api/src/five08/backend/api.pyapps/discord_bot/README.mdapps/discord_bot/src/five08/discord_bot/cogs/agent.pydocs/configuration.mdpackages/shared/src/five08/agent/__init__.pypackages/shared/src/five08/agent/orchestrator.pypackages/shared/src/five08/agent/planner.pypackages/shared/src/five08/agent/schedules.pypackages/shared/src/five08/agent/tools.pytests/unit/test_agent_cog.pytests/unit/test_agent_gateway.pytests/unit/test_agent_schedules.pytests/unit/test_backend_agent_schedules.pytests/unit/test_backend_api.py
🚧 Files skipped from review as they are similar to previous changes (12)
- packages/shared/src/five08/agent/init.py
- apps/discord_bot/README.md
- tests/unit/test_backend_agent_schedules.py
- tests/unit/test_agent_schedules.py
- docs/configuration.md
- packages/shared/src/five08/agent/planner.py
- apps/discord_bot/src/five08/discord_bot/cogs/agent.py
- packages/shared/src/five08/agent/schedules.py
- packages/shared/src/five08/agent/orchestrator.py
- packages/shared/src/five08/agent/tools.py
- tests/unit/test_backend_api.py
- apps/api/src/five08/backend/api.py
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2d829f3007
ℹ️ 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".
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_c729a9f6-569c-4a78-9ea4-a99e2654cd47) |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 043308fb1b
ℹ️ 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".
| run=run, | ||
| context=context, | ||
| ) | ||
| results = await asyncio.to_thread( |
There was a problem hiding this comment.
When several legacy frozen_actions schedules block in GitHub DNS or another synchronous provider operation, this branch bypasses _run_agent_schedule_loop_bounded and submits every run to asyncio's shared default executor without an outer timeout. The per-client deadline does not interrupt a blocked DNS lookup, so enough due schedules can consume threads used by unrelated API database work even after their worker requests time out. Route this branch through a dedicated bounded executor with a caller-visible timeout as well.
AGENTS.md reference: AGENTS.md:L95-L96
Useful? React with 👍 / 👎.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?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: bd7ff0aa2e
ℹ️ 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".
| if ( | ||
| len(_PENDING_AGENT_PLANS) >= _MAX_PENDING_AGENT_PLANS | ||
| or _pending_agent_plan_count_for_actor(context.discord_user_id) | ||
| >= _MAX_PENDING_AGENT_PLANS_PER_ACTOR | ||
| ): | ||
| return False | ||
| _PENDING_AGENT_PLANS[plan.plan_id] = (plan, context) |
There was a problem hiding this comment.
When an agent-proposed schedule requires confirmation and the backend is restarted or runs multiple replicas, the proposal exists only in this process-local dictionary. A confirmation routed to another process reaches _claim_pending_agent_plan, receives not_found, and cannot create the schedule even though the user confirms within the plan TTL. Store confirmation plans in shared durable state or provide reliable request affinity for the complete proposal/confirmation flow.
Useful? React with 👍 / 👎.
| if len(prompt) > 280: | ||
| prompt = f"{prompt[:279].rstrip()}…" |
There was a problem hiding this comment.
When the planner proposes an objective longer than 280 characters, this truncation hides the remainder from the confirmation UI: the Discord formatter displays only plan.human_summary, not the action arguments, while the full prompt of up to 4,000 characters is persisted and executed on every recurrence. An operator can therefore approve a schedule without seeing instructions placed in the hidden tail; provide an exact paged/attached preview or limit persisted prompts to what the confirmation can display.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 85cb88473a
ℹ️ 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".
| model_summary = await asyncio.to_thread( | ||
| _model_agent_schedule_summary, |
There was a problem hiding this comment.
When a frozen schedule uses model_for_public_data, this post-execution planner call runs in asyncio's shared default executor with neither the schedule bulkhead nor the remaining execution deadline. The frozen tool execution above is now bounded, but a stalled DNS or model request here continues occupying a shared thread after the worker request times out; several concurrent reports can consequently starve unrelated API database operations that also use asyncio.to_thread. Run this summary through _run_agent_schedule_sync_bounded with the same deadline, falling back to the deterministic report on timeout or capacity exhaustion.
Useful? React with 👍 / 👎.
| while not stop_event.wait(interval_seconds): | ||
| try: | ||
| if renew_job_execution_lease( | ||
| settings, | ||
| job_id, |
There was a problem hiding this comment.
When a handler remains blocked in a system call past Dramatiq's time_limit, the asynchronous interruption cannot reach the actor's finally block, but this independent heartbeat loop has no deadline and keeps advancing locked_at. Each scheduled recovery delivery therefore sees a fresh running lease and defers again, so the job is never reclaimed, never consumes another attempt, and can remain running indefinitely. Give the heartbeat the same absolute hard deadline and stop renewing before it so PostgreSQL can make the job recoverable.
AGENTS.md reference: AGENTS.md:L95-L96
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 0cebb57. Configure here.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 98fa086604
ℹ️ 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".
| default="private" if scope_type == "user" else scope_type, | ||
| ) |
There was a problem hiding this comment.
When the structured planner proposes a mismatched pair such as scope_type: "user" with visibility: "project", planner validation accepts both values independently and this execution path forwards them to MemoryFact, whose model validator rejects the combination. The user can therefore confirm a plan that deterministically fails without writing the fact; derive visibility from the scope or reject mismatched pairs during planner validation.
AGENTS.md reference: AGENTS.md:L130-L130
Useful? React with 👍 / 👎.
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_b3503ef1-2d9e-4776-8a60-f114ac90d74e) |
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_bb1358ce-c1b6-4e5b-b981-5e7e54a64029) |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fe67fcb5c9
ℹ️ 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".
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_13f4fb2e-4b2c-4caa-b8ec-1688228e2bb0) |
There was a problem hiding this comment.
💡 Codex Review
508-workflows/apps/discord_bot/src/five08/discord_bot/cogs/agent.py
Lines 1277 to 1280 in 08475fb
Expose memory IDs so users can forget facts
When a user asks what the agent remembers, this formatter displays only each fact's key and value. The only supported forget syntax in _parse_memory_action requires the opaque fact_id, and neither the remember confirmation response nor this listing exposes that ID, so a Discord user cannot construct a valid forget request for any stored fact. Include a deletion handle in the private listing or support an unambiguous key-based forget flow.
508-workflows/apps/discord_bot/src/five08/discord_bot/cogs/agent.py
Lines 154 to 155 in 08475fb
Keep confirmation controls enabled after retryable failures
When the backend returns a JSON 503 before claiming the pending plan—for example because durable confirmation storage or orchestrator initialization is temporarily unavailable—_post_backend_json returns normally, leaving transport_failed false, and this disables the confirmation view. The durable plan remains unconsumed until its TTL expires, but the requester can no longer retry it from Discord. Disable the controls only after a terminal response or treat retryable HTTP failures like transport failures.
ℹ️ 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".
| return sorted(facts, key=lambda fact: (fact.created_at, fact.id))[ | ||
| :MAX_MEMORY_FACTS_PER_LIST | ||
| ] |
There was a problem hiding this comment.
When a scope contains more than 50 facts, this ascending sort followed by a slice keeps the oldest 50 and drops every newer overflow fact. PostgresMemoryStore.list_facts instead orders descending before its limit and therefore retains the newest 50, then restores chronological order. Local/MVP behavior and tests using the in-memory store can consequently hide a newly remembered fact while production returns it; select the same newest bounded set in both implementations.
Useful? React with 👍 / 👎.
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_cb64a3e0-092d-4c50-8410-c78dfc500fc7) |
1 participant
Summary
Validation
uv run pytest -qfocused agent suite: 342 passed20260728_0100Notes
Note
High Risk
Touches agent authorization (Discord role IDs, ERP tenant gate), scheduled autonomous read-only agent runs with Discord delivery, and operational timeouts—security- and reliability-sensitive even where the diff is mostly UI and docs.
Overview
Admin dashboard gains two configuration-read surfaces: Agent schedules (list/create/pause/resume/archive/run now, stale delivery resolution, bounded read-only tool envelope copy) and Discord diagnostics (guild role catalog,
AGENT_DISCORD_*binding health, copyable env snippets without secrets).Deployment docs expand materially: per-bundle Discord role ID RBAC for
/agent/*,AGENT_ERP_ORGANIZATION_IDfail-closed ERP reads, bounded public-web planning budgets and optional SearXNG/Brave/Firecrawl, durable recurring schedule dispatcher/cleanup knobs, and worker/bot timeouts for long agent + schedule runs. GitHub agent access is documented as no access for regular Members (engineering vs steering paths).API README documents protected agent routes, schedule management semantics (fresh role snapshots, aggregate-only CRM/ERP output), and
GET /dashboard/api/discord-diagnosticsproxying the bot.Reviewed by Cursor Bugbot for commit 8f68e29. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation