fix: fail closed when spending policy check is unreachable#200
fix: fail closed when spending policy check is unreachable#200baedboibidex-cmyk wants to merge 2 commits into
Conversation
ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughThe agent gains configurable fail-open/fail-closed spending-policy enforcement via Agent spending policy enforcement
Backend agents route test updates
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Biome (2.5.0)agent/agent.jsFile contains syntax errors that prevent linting: Line 44: Illegal return statement outside of a function; Line 58: Illegal return statement outside of a function; Line 83: Illegal return statement outside of a function; Line 90: Illegal return statement outside of a function; Line 98: Illegal return statement outside of a function; Line 99: Expected a statement but instead found '} async'.; Line 107: backend/src/routes/agents.test.jsFile contains syntax errors that prevent linting: Line 145: Expected a statement but instead found '})'. 🔧 markdownlint-cli2 (0.22.1)README.mdmarkdownlint-cli2 wrapper config was not available before execution 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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/src/routes/agents.test.js (1)
172-174:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winZero-agents stats test is mocking the wrong contract function.
This test still stubs
mockListAgents, but the stats route now reads throughgetAgentCount+listAgentsPage. Please mockmockGetAgentCountas0(and optionally assertmockListAgentsPageis not called) so the test validates the current integration path.Suggested fix
it('should return zero stats when no agents', async () => { - mockListAgents.mockResolvedValueOnce([]); + mockGetAgentCount.mockResolvedValueOnce(0); const res = await request(app).get('/api/agents/stats'); expect(res.status).toBe(200); expect(res.body.totalAgents).toBe(0); expect(res.body.avgScore).toBe(0); + expect(mockListAgentsPage).not.toHaveBeenCalled(); });🤖 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 `@backend/src/routes/agents.test.js` around lines 172 - 174, The test "should return zero stats when no agents" is mocking the outdated function mockListAgents, but the stats route now uses mockGetAgentCount and mockListAgentsPage for its integration. Replace the mockListAgents.mockResolvedValueOnce([]) call with mockGetAgentCount.mockResolvedValueOnce(0) to correctly test the current code path. Optionally, add an assertion to verify that mockListAgentsPage is not called when the agent count is zero.
🤖 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 `@agent/agent.js`:
- Around line 254-271: The policy enforcement logic gated by the `if
(scoringEnabled)` condition is being skipped entirely when scoring is disabled,
bypassing deny-by-default policy checks. Decouple policy enforcement from the
scoringEnabled state by moving the checkSpend() call and associated policy
validation logic (including the checkLocalSpendLimit() fallback) outside or
independent of the `if (scoringEnabled)` block so that spending policy checks
are always performed regardless of the scoring state to ensure policies are
enforced consistently.
- Around line 299-301: Before updating sessionSpentUsdc by adding
Number(best.price_usdc), validate that the converted price is a valid number and
not NaN. If the conversion results in NaN or an invalid value, handle it as an
error case by either logging a warning, rejecting the transaction, or taking an
appropriate fail-closed action that prevents the invalid amount from poisoning
the sessionSpentUsdc accumulator and disabling subsequent daily-limit checks.
In `@README.md`:
- Line 180: The documentation is directing users to modify the `.env.example`
template file instead of the actual runtime configuration file. Change the
reference from `agent/.env.example` to `agent/.env` in the README.md line that
instructs users to set the AGENT_FAIL_OPEN environment variable. This ensures
users modify the correct file that actually controls runtime behavior,
preventing confusion during setup and verification.
---
Outside diff comments:
In `@backend/src/routes/agents.test.js`:
- Around line 172-174: The test "should return zero stats when no agents" is
mocking the outdated function mockListAgents, but the stats route now uses
mockGetAgentCount and mockListAgentsPage for its integration. Replace the
mockListAgents.mockResolvedValueOnce([]) call with
mockGetAgentCount.mockResolvedValueOnce(0) to correctly test the current code
path. Optionally, add an assertion to verify that mockListAgentsPage is not
called when the agent count is zero.
🪄 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: f1a5eff8-4ca6-40d4-882a-d7cc6a6d80dc
📒 Files selected for processing (5)
README.mdagent/.env.exampleagent/agent.jsbackend/src/lib/contract.test.jsbackend/src/routes/agents.test.js
| if (scoringEnabled) { | ||
| const check = await checkSpend(best.price_usdc, category); | ||
|
|
||
| if (!check.allowed) { | ||
| logger.warn(`${tag()} Payment blocked by spending policy: ${check.reason}`); | ||
| return; | ||
| } | ||
| logger.info(`${tag()} Spending policy check passed`); | ||
|
|
||
| if (check.reason === 'Policy check unreachable (fail-open enabled)') { | ||
| const localCheck = checkLocalSpendLimit(best.price_usdc); | ||
| if (!localCheck.allowed) { | ||
| logger.warn(`${tag()} Payment blocked by local fallback spending policy: ${localCheck.reason}`); | ||
| return; | ||
| } | ||
| logger.warn(`${tag()} Spending policy check unreachable; using local fallback guard for this session`); | ||
| } else { | ||
| logger.info(`${tag()} Spending policy check passed`); | ||
| } |
There was a problem hiding this comment.
Policy enforcement is bypassed when scoringEnabled is false.
checkSpend() is currently gated by if (scoringEnabled). When registration/policy backend is unreachable (or contract is not configured), this block is skipped and payments proceed without deny-by-default or local fallback checks.
Suggested fix (decouple policy enforcement from scoring state)
- // Spending policy check
- if (scoringEnabled) {
- const check = await checkSpend(best.price_usdc, category);
+ // Spending policy check (always enforce)
+ {
+ const check = await checkSpend(best.price_usdc, category);
if (!check.allowed) {
logger.warn(`${tag()} Payment blocked by spending policy: ${check.reason}`);
return;
}
if (check.reason === 'Policy check unreachable (fail-open enabled)') {
const localCheck = checkLocalSpendLimit(best.price_usdc);
if (!localCheck.allowed) {
logger.warn(`${tag()} Payment blocked by local fallback spending policy: ${localCheck.reason}`);
return;
}
logger.warn(`${tag()} Spending policy check unreachable; using local fallback guard for this session`);
} else {
logger.info(`${tag()} Spending policy check passed`);
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (scoringEnabled) { | |
| const check = await checkSpend(best.price_usdc, category); | |
| if (!check.allowed) { | |
| logger.warn(`${tag()} Payment blocked by spending policy: ${check.reason}`); | |
| return; | |
| } | |
| logger.info(`${tag()} Spending policy check passed`); | |
| if (check.reason === 'Policy check unreachable (fail-open enabled)') { | |
| const localCheck = checkLocalSpendLimit(best.price_usdc); | |
| if (!localCheck.allowed) { | |
| logger.warn(`${tag()} Payment blocked by local fallback spending policy: ${localCheck.reason}`); | |
| return; | |
| } | |
| logger.warn(`${tag()} Spending policy check unreachable; using local fallback guard for this session`); | |
| } else { | |
| logger.info(`${tag()} Spending policy check passed`); | |
| } | |
| // Spending policy check (always enforce) | |
| { | |
| const check = await checkSpend(best.price_usdc, category); | |
| if (!check.allowed) { | |
| logger.warn(`${tag()} Payment blocked by spending policy: ${check.reason}`); | |
| return; | |
| } | |
| if (check.reason === 'Policy check unreachable (fail-open enabled)') { | |
| const localCheck = checkLocalSpendLimit(best.price_usdc); | |
| if (!localCheck.allowed) { | |
| logger.warn(`${tag()} Payment blocked by local fallback spending policy: ${localCheck.reason}`); | |
| return; | |
| } | |
| logger.warn(`${tag()} Spending policy check unreachable; using local fallback guard for this session`); | |
| } else { | |
| logger.info(`${tag()} Spending policy check 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 `@agent/agent.js` around lines 254 - 271, The policy enforcement logic gated by
the `if (scoringEnabled)` condition is being skipped entirely when scoring is
disabled, bypassing deny-by-default policy checks. Decouple policy enforcement
from the scoringEnabled state by moving the checkSpend() call and associated
policy validation logic (including the checkLocalSpendLimit() fallback) outside
or independent of the `if (scoringEnabled)` block so that spending policy checks
are always performed regardless of the scoring state to ensure policies are
enforced consistently.
| - Pay via x402 on Stellar | ||
| - Log the data received and update on-chain reputation | ||
|
|
||
| If you explicitly want development-only fail-open behavior, set `AGENT_FAIL_OPEN=true` in [agent/.env.example](agent/.env.example). The agent will still enforce a per-process local fallback limit for the current run. |
There was a problem hiding this comment.
Point fail-open setup to the runtime .env, not .env.example.
This currently instructs users to edit the template file, which can leave runtime behavior unchanged and cause confusion during verification.
Suggested doc fix
-If you explicitly want development-only fail-open behavior, set `AGENT_FAIL_OPEN=true` in [agent/.env.example](agent/.env.example). The agent will still enforce a per-process local fallback limit for the current run.
+If you explicitly want development-only fail-open behavior, set `AGENT_FAIL_OPEN=true` in `agent/.env` (copied from [agent/.env.example](agent/.env.example)). The agent will still enforce a per-process local fallback limit for the current run.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| If you explicitly want development-only fail-open behavior, set `AGENT_FAIL_OPEN=true` in [agent/.env.example](agent/.env.example). The agent will still enforce a per-process local fallback limit for the current run. | |
| If you explicitly want development-only fail-open behavior, set `AGENT_FAIL_OPEN=true` in `agent/.env` (copied from [agent/.env.example](agent/.env.example)). The agent will still enforce a per-process local fallback limit for the current run. |
🤖 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 `@README.md` at line 180, The documentation is directing users to modify the
`.env.example` template file instead of the actual runtime configuration file.
Change the reference from `agent/.env.example` to `agent/.env` in the README.md
line that instructs users to set the AGENT_FAIL_OPEN environment variable. This
ensures users modify the correct file that actually controls runtime behavior,
preventing confusion during setup and verification.
ritik4ever
left a comment
ritik4ever
left a comment
There was a problem hiding this comment.
Thanks for the contribution! The fail-closed approach is a good improvement, but before we merge could you please address the critical review comment?
It appears the spending policy check is still gated by scoringEnabled, which may allow payments to proceed without policy enforcement when scoring is disabled or unavailable. Since this PR is intended to enforce fail-closed behavior, that path should also be protected.
Additionally, there are currently merge conflicts that need to be resolved before we can merge.
Once those are addressed, I'll take another look. Thanks!
ritik4ever
left a comment
ritik4ever
left a comment
There was a problem hiding this comment.
Thanks for the contribution. Before we can merge, I still need to see the critical review comment addressed.
The spending-policy enforcement appears to remain gated by scoringEnabled, which means payments may proceed without policy checks when scoring is disabled or unavailable. Since this PR is intended to enforce fail-closed behavior, that path also needs to be protected.
There are also unresolved merge conflicts in:
agent/.env.exampleagent/agent.jsbackend/src/routes/agents.test.js
Additionally, please consider hardening the session spend tracking against invalid/NaN payment amounts so the local fallback limits cannot be bypassed.
Once those items are addressed, I'll review again.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
agent/agent.js (1)
37-96: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winRestore the
ensureRegistered()wrapper around this block.This
try/returnsequence is sitting at module scope, which is why the parser is flagging illegal top-levelreturns here. Re-wrap the registration flow in its async function before merge.🤖 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 `@agent/agent.js` around lines 37 - 96, The registration flow in the agent module is currently sitting at module scope, which leaves the `try`/`return` sequence outside any function. Move this whole block back inside the async `ensureRegistered()` wrapper, and keep the existing fetch/error handling and logger calls (`EVENT.AGENT_REGISTERED`, `logger.info`, `logger.warn`) within that function so the top-level `return`s are no longer illegal.Source: Linters/SAST tools
backend/src/routes/agents.test.js (1)
71-78: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winMount the freshly imported router onto a new app instance.
After
vi.resetModules(), Line 76 imports a new router, but the shown setup never reassignsappor mounts that router. That means these tests either keep exercising a stale router instance from earlier setup orrequest(app)runs againstundefined, so the per-test reset is ineffective.Suggested fix
beforeEach(async () => { vi.clearAllMocks(); vi.resetModules(); resetIdempotencyStore(); const router = (await import('./agents.js')).default; + app = makeApp(); + app.use(router); });🤖 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 `@backend/src/routes/agents.test.js` around lines 71 - 78, The test setup in agents.test.js imports a fresh router after vi.resetModules(), but it never mounts that router onto the app used by request(app), so the per-test reset is ineffective. Update the beforeEach setup to rebuild the app instance and attach the newly imported default router from agents.js before each test, using the existing app/request test harness so each test runs against the current router state.
🤖 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 `@agent/agent.js`:
- Around line 274-352: The payment handling in runTask() is missing the
enclosing candidate-attempt retry/selection loop, so the existing references to
selected, failed, and continue are invalid and break control flow. Restore the
loop that iterates over candidate services before the payment request block, and
keep the current success/failure branches inside it so selected is always
defined and failed tracking works correctly. Use runTask(), selected, failed,
and the payment success/error logging blocks as the anchors when reintroducing
the retry logic.
In `@backend/src/routes/agents.test.js`:
- Around line 80-84: The POST /admin/agents/:address/flag test suite is being
closed too early by a stray describe terminator, which leaves the flag tests
outside the suite and breaks parsing. Remove the extra closing bracket in the
agents.test.js flag block so the describe for POST /admin/agents/:address/flag
stays open until its intended it(...) cases, and keep the suite structure
consistent with the surrounding test blocks.
- Around line 7-35: The test mock for contract.js is missing the admin-specific
exports that agents.js imports, so the router setup breaks before the tests run.
Add dedicated mocks for flagAgentOnChain and adminDeactivateAgentOnChain in this
vi.mock block, and update the related assertions to use those same mock symbols
instead of the unused deactivateAgentOnChain mock so the import surface matches
what agents.js actually consumes.
---
Outside diff comments:
In `@agent/agent.js`:
- Around line 37-96: The registration flow in the agent module is currently
sitting at module scope, which leaves the `try`/`return` sequence outside any
function. Move this whole block back inside the async `ensureRegistered()`
wrapper, and keep the existing fetch/error handling and logger calls
(`EVENT.AGENT_REGISTERED`, `logger.info`, `logger.warn`) within that function so
the top-level `return`s are no longer illegal.
In `@backend/src/routes/agents.test.js`:
- Around line 71-78: The test setup in agents.test.js imports a fresh router
after vi.resetModules(), but it never mounts that router onto the app used by
request(app), so the per-test reset is ineffective. Update the beforeEach setup
to rebuild the app instance and attach the newly imported default router from
agents.js before each test, using the existing app/request test harness so each
test runs against the current router state.
🪄 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: b3438cf8-71f3-48e0-99bf-816fa779aef5
📒 Files selected for processing (4)
README.mdagent/.env.exampleagent/agent.jsbackend/src/routes/agents.test.js
✅ Files skipped from review due to trivial changes (1)
- README.md
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
agent/agent.js (1)
37-96: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winRestore the
ensureRegistered()wrapper around this block.This
try/returnsequence is sitting at module scope, which is why the parser is flagging illegal top-levelreturns here. Re-wrap the registration flow in its async function before merge.🤖 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 `@agent/agent.js` around lines 37 - 96, The registration flow in the agent module is currently sitting at module scope, which leaves the `try`/`return` sequence outside any function. Move this whole block back inside the async `ensureRegistered()` wrapper, and keep the existing fetch/error handling and logger calls (`EVENT.AGENT_REGISTERED`, `logger.info`, `logger.warn`) within that function so the top-level `return`s are no longer illegal.Source: Linters/SAST tools
backend/src/routes/agents.test.js (1)
71-78: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winMount the freshly imported router onto a new app instance.
After
vi.resetModules(), Line 76 imports a new router, but the shown setup never reassignsappor mounts that router. That means these tests either keep exercising a stale router instance from earlier setup orrequest(app)runs againstundefined, so the per-test reset is ineffective.Suggested fix
beforeEach(async () => { vi.clearAllMocks(); vi.resetModules(); resetIdempotencyStore(); const router = (await import('./agents.js')).default; + app = makeApp(); + app.use(router); });🤖 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 `@backend/src/routes/agents.test.js` around lines 71 - 78, The test setup in agents.test.js imports a fresh router after vi.resetModules(), but it never mounts that router onto the app used by request(app), so the per-test reset is ineffective. Update the beforeEach setup to rebuild the app instance and attach the newly imported default router from agents.js before each test, using the existing app/request test harness so each test runs against the current router state.
🤖 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 `@agent/agent.js`:
- Around line 274-352: The payment handling in runTask() is missing the
enclosing candidate-attempt retry/selection loop, so the existing references to
selected, failed, and continue are invalid and break control flow. Restore the
loop that iterates over candidate services before the payment request block, and
keep the current success/failure branches inside it so selected is always
defined and failed tracking works correctly. Use runTask(), selected, failed,
and the payment success/error logging blocks as the anchors when reintroducing
the retry logic.
In `@backend/src/routes/agents.test.js`:
- Around line 80-84: The POST /admin/agents/:address/flag test suite is being
closed too early by a stray describe terminator, which leaves the flag tests
outside the suite and breaks parsing. Remove the extra closing bracket in the
agents.test.js flag block so the describe for POST /admin/agents/:address/flag
stays open until its intended it(...) cases, and keep the suite structure
consistent with the surrounding test blocks.
- Around line 7-35: The test mock for contract.js is missing the admin-specific
exports that agents.js imports, so the router setup breaks before the tests run.
Add dedicated mocks for flagAgentOnChain and adminDeactivateAgentOnChain in this
vi.mock block, and update the related assertions to use those same mock symbols
instead of the unused deactivateAgentOnChain mock so the import surface matches
what agents.js actually consumes.
---
Outside diff comments:
In `@agent/agent.js`:
- Around line 37-96: The registration flow in the agent module is currently
sitting at module scope, which leaves the `try`/`return` sequence outside any
function. Move this whole block back inside the async `ensureRegistered()`
wrapper, and keep the existing fetch/error handling and logger calls
(`EVENT.AGENT_REGISTERED`, `logger.info`, `logger.warn`) within that function so
the top-level `return`s are no longer illegal.
In `@backend/src/routes/agents.test.js`:
- Around line 71-78: The test setup in agents.test.js imports a fresh router
after vi.resetModules(), but it never mounts that router onto the app used by
request(app), so the per-test reset is ineffective. Update the beforeEach setup
to rebuild the app instance and attach the newly imported default router from
agents.js before each test, using the existing app/request test harness so each
test runs against the current router state.
🪄 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: b3438cf8-71f3-48e0-99bf-816fa779aef5
📒 Files selected for processing (4)
README.mdagent/.env.exampleagent/agent.jsbackend/src/routes/agents.test.js
✅ Files skipped from review due to trivial changes (1)
- README.md
🛑 Comments failed to post (3)
agent/agent.js (1)
274-352: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Reintroduce the missing retry/selection block in
runTask().The payment path still references
selected,failed, andcontinue, but there is no enclosing candidate-attempt loop after Line 273. This is the root cause of the parse errors here and the broken success/failure control flow.🧰 Tools
🪛 Biome (2.5.0)
[error] 302-302: A
continuestatement can only be used within an enclosingfor,whileordo whilestatement.(parse)
[error] 322-322: A
continuestatement can only be used within an enclosingfor,whileordo whilestatement.(parse)
🤖 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 `@agent/agent.js` around lines 274 - 352, The payment handling in runTask() is missing the enclosing candidate-attempt retry/selection loop, so the existing references to selected, failed, and continue are invalid and break control flow. Restore the loop that iterates over candidate services before the payment request block, and keep the current success/failure branches inside it so selected is always defined and failed tracking works correctly. Use runTask(), selected, failed, and the payment success/error logging blocks as the anchors when reintroducing the retry logic.Source: Linters/SAST tools
backend/src/routes/agents.test.js (2)
7-35: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Mock the admin contract symbols the router actually imports.
agents.jsimportsflagAgentOnChainandadminDeactivateAgentOnChain, but this mock only wiresdeactivateAgentOnChain, and the file never definesmockFlagAgentOnChain/mockAdminDeactivateAgentOnChain. Line 76 will fail when./agents.jsis imported, and the assertions later in this file also dereference undefined mocks.Suggested fix
const mockListAgentsPage = vi.fn(); const mockRegisterAgentOnChain = vi.fn(); const mockRecordPaymentOnChain = vi.fn(); +const mockFlagAgentOnChain = vi.fn(); const mockDeactivateAgentOnChain = vi.fn(); +const mockAdminDeactivateAgentOnChain = vi.fn(); const mockUpdatePolicyOnChain = vi.fn(); const mockBuildUnsignedAgentTx = vi.fn(); const mockSubmitSignedAgentTx = vi.fn(); vi.mock('../lib/contract.js', () => ({ getAgent: (...args) => mockGetAgent(...args), getAgentPolicy: (...args) => mockGetAgentPolicy(...args), getAgentScore: (...args) => mockGetAgentScore(...args), getAgentCount: (...args) => mockGetAgentCount(...args), isAgentEligible: (...args) => mockIsAgentEligible(...args), checkSpendingAllowed: (...args) => mockCheckSpendingAllowed(...args), listAgentsPage: (...args) => mockListAgentsPage(...args), registerAgentOnChain: (...args) => mockRegisterAgentOnChain(...args), recordPaymentOnChain: (...args) => mockRecordPaymentOnChain(...args), + flagAgentOnChain: (...args) => mockFlagAgentOnChain(...args), deactivateAgentOnChain: (...args) => mockDeactivateAgentOnChain(...args), + adminDeactivateAgentOnChain: (...args) => mockAdminDeactivateAgentOnChain(...args), updatePolicyOnChain: (...args) => mockUpdatePolicyOnChain(...args), buildUnsignedAgentTx: (...args) => mockBuildUnsignedAgentTx(...args), submitSignedAgentTx: (...args) => mockSubmitSignedAgentTx(...args), }));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.const mockGetAgent = vi.fn(); const mockGetAgentPolicy = vi.fn(); const mockGetAgentScore = vi.fn(); const mockGetAgentCount = vi.fn(); const mockIsAgentEligible = vi.fn(); const mockCheckSpendingAllowed = vi.fn(); const mockListAgentsPage = vi.fn(); const mockRegisterAgentOnChain = vi.fn(); const mockRecordPaymentOnChain = vi.fn(); const mockFlagAgentOnChain = vi.fn(); const mockDeactivateAgentOnChain = vi.fn(); const mockAdminDeactivateAgentOnChain = vi.fn(); const mockUpdatePolicyOnChain = vi.fn(); const mockBuildUnsignedAgentTx = vi.fn(); const mockSubmitSignedAgentTx = vi.fn(); vi.mock('../lib/contract.js', () => ({ getAgent: (...args) => mockGetAgent(...args), getAgentPolicy: (...args) => mockGetAgentPolicy(...args), getAgentScore: (...args) => mockGetAgentScore(...args), getAgentCount: (...args) => mockGetAgentCount(...args), isAgentEligible: (...args) => mockIsAgentEligible(...args), checkSpendingAllowed: (...args) => mockCheckSpendingAllowed(...args), listAgentsPage: (...args) => mockListAgentsPage(...args), registerAgentOnChain: (...args) => mockRegisterAgentOnChain(...args), recordPaymentOnChain: (...args) => mockRecordPaymentOnChain(...args), flagAgentOnChain: (...args) => mockFlagAgentOnChain(...args), deactivateAgentOnChain: (...args) => mockDeactivateAgentOnChain(...args), adminDeactivateAgentOnChain: (...args) => mockAdminDeactivateAgentOnChain(...args), updatePolicyOnChain: (...args) => mockUpdatePolicyOnChain(...args), buildUnsignedAgentTx: (...args) => mockBuildUnsignedAgentTx(...args), submitSignedAgentTx: (...args) => mockSubmitSignedAgentTx(...args),🤖 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 `@backend/src/routes/agents.test.js` around lines 7 - 35, The test mock for contract.js is missing the admin-specific exports that agents.js imports, so the router setup breaks before the tests run. Add dedicated mocks for flagAgentOnChain and adminDeactivateAgentOnChain in this vi.mock block, and update the related assertions to use those same mock symbols instead of the unused deactivateAgentOnChain mock so the import surface matches what agents.js actually consumes.
80-84: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Remove the stray
});that closes the flag suite early.Line 84 closes the
describebefore anyit(...)blocks. That matches the parse failure reported later in the file and leaves the flag tests outside the suite.Suggested fix
describe('POST /admin/agents/:address/flag', () => { const ADDRESS = 'GAMASX3TLJIDO42FO3GTX7IQAYN7RJ4U4CXJOROTB7RSV3NGPUEIEQH3'; - - }); it('flags an agent with valid admin key', async () => {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.describe('POST /admin/agents/:address/flag', () => { const ADDRESS = 'GAMASX3TLJIDO42FO3GTX7IQAYN7RJ4U4CXJOROTB7RSV3NGPUEIEQH3';🤖 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 `@backend/src/routes/agents.test.js` around lines 80 - 84, The POST /admin/agents/:address/flag test suite is being closed too early by a stray describe terminator, which leaves the flag tests outside the suite and breaks parsing. Remove the extra closing bracket in the agents.test.js flag block so the describe for POST /admin/agents/:address/flag stays open until its intended it(...) cases, and keep the suite structure consistent with the surrounding test blocks.Source: Linters/SAST tools
2 participants
close #126
Summary
This PR fixes the agent-side spending policy flow so that any failed or unreachable
can-spendcheck now blocks the payment flow by default instead of silently allowing it.What changed
MAX_PER_TX/MAX_PER_DAYbounds during backend outages.Acceptance criteria checklist
AGENT_FAIL_OPEN=true.Verification
cd /workspaces/lodestar/backend && npm run testnode --check /workspaces/lodestar/agent/agent.jsNotes
AGENT_FAIL_OPENsetting is documented as development-only and should not be used in production.Summary by CodeRabbit
New Features
Bug Fixes