Security: Make scoring a hard requirement by default#217
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:
📝 WalkthroughWalkthroughAdds required scoring startup enforcement, verifies registration before enabling scoring, expands payment and outcome handling in the agent runtime, updates tests for the new behavior, and changes CI dependency install commands. ChangesScoring enforcement and registration hardening
Workflow dependency install changes
Estimated code review effort: 4 (Complex) | ~45 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)
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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
agent/agent.js (1)
94-96: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftThe new policy env vars are no-ops against the current register API.
POST /api/agents/registerinbackend/src/routes/agents.js:270-299only consumesagentAddress,name, anddescription, somaxPerTxUsdc,maxPerDayUsdc, andallowedCategoriesare dropped here. That means the newly documented env knobs do not actually configure the registered agent’s policy/categories.Either extend the backend contract to persist these fields end-to-end, or remove/document them as non-functional until that contract exists.
🤖 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 94 - 96, The new policy env vars in agent registration are currently no-ops because the register flow only sends agentAddress, name, and description. Update the agent registration path in agent/agent.js (and the matching backend register handler in backend/src/routes/agents.js) so maxPerTxUsdc, maxPerDayUsdc, and allowedCategories are accepted and persisted end-to-end, or remove them from the request/documentation until the API contract supports them.
🧹 Nitpick comments (1)
agent/agent.test.js (1)
245-245: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPlease keep one
main()summary test active.This PR changes
main(), but bothAGENT_COMPLETEassertions are now skipped. That leaves regressions insuccessCount/failCount/scoreDeltainvisible on the exact path modified here. Re-scoping one lightweight summary test around the mocked no-services flow would preserve coverage without depending on the brokenrunTaskhappy path.Also applies to: 283-283
🤖 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.test.js` at line 245, Keep at least one main() summary assertion active in agent.test.js by unskipping or re-scoping one lightweight AGENT_COMPLETE test around the mocked no-services flow. The issue is that both summary checks are currently skipped, so changes to main() are not covering successCount, failCount, and scoreDelta. Update the existing logs agent_complete test (and the other skipped AGENT_COMPLETE case) so one of them still exercises the summary payload without relying on the broken runTask happy 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.
Inline comments:
In `@agent/agent.js`:
- Around line 118-124: The registration failure path in agent.js currently
treats every non-OK response as fatal, but a 409 with code ALREADY_EXISTS should
be handled as an already-registered agent. Update the registration logic around
the regRes/err handling so that when status is 409 and err.code is
ALREADY_EXISTS, it performs the same verification flow as a successful
registration by fetching the agent with the existing GET /api/agents/:address
check and only falling back to failure if that verification fails. Keep the
existing logger.error path for true registration errors, and make sure
AGENT_REGISTERED and the scoring-disabled flag are set based on the verification
result rather than the raw POST response.
---
Outside diff comments:
In `@agent/agent.js`:
- Around line 94-96: The new policy env vars in agent registration are currently
no-ops because the register flow only sends agentAddress, name, and description.
Update the agent registration path in agent/agent.js (and the matching backend
register handler in backend/src/routes/agents.js) so maxPerTxUsdc,
maxPerDayUsdc, and allowedCategories are accepted and persisted end-to-end, or
remove them from the request/documentation until the API contract supports them.
---
Nitpick comments:
In `@agent/agent.test.js`:
- Line 245: Keep at least one main() summary assertion active in agent.test.js
by unskipping or re-scoping one lightweight AGENT_COMPLETE test around the
mocked no-services flow. The issue is that both summary checks are currently
skipped, so changes to main() are not covering successCount, failCount, and
scoreDelta. Update the existing logs agent_complete test (and the other skipped
AGENT_COMPLETE case) so one of them still exercises the summary payload without
relying on the broken runTask happy path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: a4758391-7c07-41ea-a53e-06dec8796e98
📒 Files selected for processing (3)
agent/.env.exampleagent/agent.jsagent/agent.test.js
| } | ||
| const err = await regRes.json().catch(() => ({})); | ||
| logger.warn( | ||
| { event: EVENT.AGENT_REGISTERED, agentAddress: AGENT_ADDRESS, scoringEnabled: false, err }, | ||
| 'Registration failed — scoring disabled' | ||
| logger.error( | ||
| { event: EVENT.AGENT_REGISTERED, agentAddress: AGENT_ADDRESS, scoringEnabled: false, err, status: regRes.status }, | ||
| 'Registration call failed — scoring disabled' | ||
| ); | ||
| return false; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Handle 409 ALREADY_EXISTS by verifying registration instead of failing.
The backend explicitly returns 409 { code: 'ALREADY_EXISTS' } when another process already registered the agent. This branch currently logs "Registration call failed" and returns false, so AGENT_REQUIRE_SCORING=true can shut the agent down even though a follow-up GET /api/agents/:address would succeed.
Suggested direction
}
const err = await regRes.json().catch(() => ({}));
+ if (regRes.status === 409 && err.code === 'ALREADY_EXISTS') {
+ const verifyRes = await fetch(`${LODESTAR_API_URL}/api/agents/${AGENT_ADDRESS}`);
+ if (verifyRes.ok) {
+ const verifyData = await verifyRes.json();
+ const verifyAgent = verifyData.agent ?? verifyData;
+ currentScore = verifyAgent.score ?? 100;
+ logger.info(
+ { event: EVENT.AGENT_REGISTERED, agentAddress: AGENT_ADDRESS, score: currentScore, scoringEnabled: true },
+ 'Already registered during registration retry'
+ );
+ return true;
+ }
+ }
logger.error(
{ event: EVENT.AGENT_REGISTERED, agentAddress: AGENT_ADDRESS, scoringEnabled: false, err, status: regRes.status },
'Registration call failed — scoring disabled'
);📝 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 err = await regRes.json().catch(() => ({})); | |
| logger.warn( | |
| { event: EVENT.AGENT_REGISTERED, agentAddress: AGENT_ADDRESS, scoringEnabled: false, err }, | |
| 'Registration failed — scoring disabled' | |
| logger.error( | |
| { event: EVENT.AGENT_REGISTERED, agentAddress: AGENT_ADDRESS, scoringEnabled: false, err, status: regRes.status }, | |
| 'Registration call failed — scoring disabled' | |
| ); | |
| return false; | |
| } | |
| const err = await regRes.json().catch(() => ({})); | |
| if (regRes.status === 409 && err.code === 'ALREADY_EXISTS') { | |
| const verifyRes = await fetch(`${LODESTAR_API_URL}/api/agents/${AGENT_ADDRESS}`); | |
| if (verifyRes.ok) { | |
| const verifyData = await verifyRes.json(); | |
| const verifyAgent = verifyData.agent ?? verifyData; | |
| currentScore = verifyAgent.score ?? 100; | |
| logger.info( | |
| { event: EVENT.AGENT_REGISTERED, agentAddress: AGENT_ADDRESS, score: currentScore, scoringEnabled: true }, | |
| 'Already registered during registration retry' | |
| ); | |
| return true; | |
| } | |
| } | |
| logger.error( | |
| { event: EVENT.AGENT_REGISTERED, agentAddress: AGENT_ADDRESS, scoringEnabled: false, err, status: regRes.status }, | |
| 'Registration call failed — scoring disabled' | |
| ); | |
| return false; |
🤖 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 118 - 124, The registration failure path in
agent.js currently treats every non-OK response as fatal, but a 409 with code
ALREADY_EXISTS should be handled as an already-registered agent. Update the
registration logic around the regRes/err handling so that when status is 409 and
err.code is ALREADY_EXISTS, it performs the same verification flow as a
successful registration by fetching the agent with the existing GET
/api/agents/:address check and only falling back to failure if that verification
fails. Keep the existing logger.error path for true registration errors, and
make sure AGENT_REGISTERED and the scoring-disabled flag are set based on the
verification result rather than the raw POST response.
ritik4ever
left a comment
ritik4ever
left a comment
There was a problem hiding this comment.
Thanks for the contribution! I like the direction of making scoring a required prerequisite by default and adding explicit registration verification.
Before we merge, could you please address one important edge case?
When the backend returns 409 ALREADY_EXISTS, the registration request has failed because the agent is already registered—not because scoring is unavailable. With AGENT_REQUIRE_SCORING=true, the current logic appears to treat this as a hard failure and exits the agent.
Instead, could you handle ALREADY_EXISTS by performing the registration verification step? If the subsequent lookup confirms the agent is registered, startup should continue; otherwise it should fail closed as intended.
Also, there are currently merge conflicts that will need to be resolved before this can be merged.
Once those are addressed, I'm happy to take another look.
|
@Samaro1 any update |
Sorry, I'll implement the changes requested and push tomorrow. My laptop is off currently |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
agent/agent.test.js (1)
28-32: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winMake the x402 mock return a sentinel header.
Returning
{}keeps tests green even ifrunTask()drops payment headers. Use a recognizable header value and assert the endpointfetchreceives it.🧪 Proposed direction
x402HTTPClient: class { encodePaymentSignatureHeader() { - return {}; + return { 'x-payment': 'signed-test-payment' }; } },🤖 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.test.js` around lines 28 - 32, The x402HTTPClient mock currently returns an empty object, which can hide regressions in payment header propagation. Update the mock in agent.test.js so encodePaymentSignatureHeader() returns a recognizable sentinel header value, then add/adjust the assertions around runTask() to verify the endpoint fetch call receives that header via the x402HTTPClient mock.
🤖 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 236-238: The fetchServices() request builds the services URL with
an unescaped category query string, so update the fetch call to encode the
category value before interpolating it into the URL. Use the
fetchServices(category) helper to locate the change and ensure the request to
LODESTAR_API_URL keeps the same behavior while safely handling spaces and
reserved characters in category.
- Around line 140-146: The spend-policy check currently fails open in
`checkSpend`, allowing `{ allowed: true }` on non-OK responses and fetch errors;
change this to fail closed when enforcement is required by returning `{ allowed:
false, reason: 'Spend policy unavailable' }` for Lodestar outages/500s. Update
the `checkSpend` logic in `agent/agent.js` to keep the permissive path only when
scoring enforcement is not enabled, and ensure the `AGENT_REQUIRE_SCORING` flow
blocks spending until an authoritative decision is available.
---
Nitpick comments:
In `@agent/agent.test.js`:
- Around line 28-32: The x402HTTPClient mock currently returns an empty object,
which can hide regressions in payment header propagation. Update the mock in
agent.test.js so encodePaymentSignatureHeader() returns a recognizable sentinel
header value, then add/adjust the assertions around runTask() to verify the
endpoint fetch call receives that header via the x402HTTPClient mock.
🪄 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: 3bae9d80-9d45-4da0-aef1-e348c1224617
📒 Files selected for processing (2)
agent/agent.jsagent/agent.test.js
| async function fetchServices(category) { | ||
| const res = await fetch(`${LODESTAR_API_URL}/api/services?category=${category}`); | ||
| if (!res.ok) throw new Error(`Registry fetch failed: ${res.status}`); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Encode the category query parameter.
category is environment/config driven; values containing spaces, &, or = will produce a malformed registry request or change the query shape.
🔧 Proposed fix
async function fetchServices(category) {
- const res = await fetch(`${LODESTAR_API_URL}/api/services?category=${category}`);
+ const res = await fetch(`${LODESTAR_API_URL}/api/services?category=${encodeURIComponent(category)}`);📝 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.
| async function fetchServices(category) { | |
| const res = await fetch(`${LODESTAR_API_URL}/api/services?category=${category}`); | |
| if (!res.ok) throw new Error(`Registry fetch failed: ${res.status}`); | |
| async function fetchServices(category) { | |
| const res = await fetch(`${LODESTAR_API_URL}/api/services?category=${encodeURIComponent(category)}`); | |
| if (!res.ok) throw new Error(`Registry fetch failed: ${res.status}`); |
🤖 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 236 - 238, The fetchServices() request builds
the services URL with an unescaped category query string, so update the fetch
call to encode the category value before interpolating it into the URL. Use the
fetchServices(category) helper to locate the change and ensure the request to
LODESTAR_API_URL keeps the same behavior while safely handling spaces and
reserved characters in category.
Samaro1
force-pushed
the
security/require-scoring-hard-failure
branch
from
7b9eb5e to
4cdbc9a
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/.env.example`:
- Around line 18-32: Resolve the merge conflict in agent/.env.example by
removing the conflict markers and keeping the intended example settings from
both sides. Preserve the service selection entries such as
AGENT_MIN_SERVICE_REPUTATION and AGENT_MAX_SERVICE_RETRIES, and include the new
AGENT_REQUIRE_SCORING documentation/value from the security change so the dotenv
example remains valid and complete.
In `@agent/agent.js`:
- Around line 196-211: Resolve the merge conflict in agent/agent.js by removing
all conflict markers (`<<<<<<<`, `=======`, `>>>>>>>`) and keeping the correct
implementation for each conflicted block. Make sure the `buildHttpClient`
function and the `dispose` export both remain properly defined and in valid
scope, then verify the rest of the conflicted regions in the module are
similarly cleaned up so the file parses again.
- Around line 498-500: The runTask invocation is still passing a stale
httpClient argument even though runTask(category, buildUrl, scoringEnabled) now
creates its own client; update the call site in the task loop to pass only
category, buildUrl, and scoringEnabled. Use the runTask function and the
surrounding tasks iteration in agent.js to locate and remove the extra argument
so the code no longer depends on an undefined binding.
🪄 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: 96b2f561-73f7-43fe-ae58-4f87dc61b56d
📒 Files selected for processing (3)
agent/.env.exampleagent/agent.jsagent/agent.test.js
🚧 Files skipped from review as they are similar to previous changes (1)
- agent/agent.test.js
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
🤖 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/.env.example`:
- Around line 18-32: Resolve the merge conflict in agent/.env.example by
removing the conflict markers and keeping the intended example settings from
both sides. Preserve the service selection entries such as
AGENT_MIN_SERVICE_REPUTATION and AGENT_MAX_SERVICE_RETRIES, and include the new
AGENT_REQUIRE_SCORING documentation/value from the security change so the dotenv
example remains valid and complete.
In `@agent/agent.js`:
- Around line 196-211: Resolve the merge conflict in agent/agent.js by removing
all conflict markers (`<<<<<<<`, `=======`, `>>>>>>>`) and keeping the correct
implementation for each conflicted block. Make sure the `buildHttpClient`
function and the `dispose` export both remain properly defined and in valid
scope, then verify the rest of the conflicted regions in the module are
similarly cleaned up so the file parses again.
- Around line 498-500: The runTask invocation is still passing a stale
httpClient argument even though runTask(category, buildUrl, scoringEnabled) now
creates its own client; update the call site in the task loop to pass only
category, buildUrl, and scoringEnabled. Use the runTask function and the
surrounding tasks iteration in agent.js to locate and remove the extra argument
so the code no longer depends on an undefined binding.
🪄 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: 96b2f561-73f7-43fe-ae58-4f87dc61b56d
📒 Files selected for processing (3)
agent/.env.exampleagent/agent.jsagent/agent.test.js
🚧 Files skipped from review as they are similar to previous changes (1)
- agent/agent.test.js
🛑 Comments failed to post (3)
agent/.env.example (1)
18-32: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Resolve the merge conflict before merging.
agent/.env.examplestill contains conflict markers, so the file is invalid dotenv syntax and the example config is broken as-is.🧰 Tools
🪛 dotenv-linter (4.0.0)
[warning] 18-18: [KeyWithoutValue] The <<<<<<< HEAD key should be with a value or have an equal sign
(KeyWithoutValue)
[warning] 18-18: [LeadingCharacter] Invalid leading character detected
(LeadingCharacter)
[warning] 23-23: [UnorderedKey] The AGENT_MAX_SERVICE_RETRIES key should go before the AGENT_MIN_SERVICE_REPUTATION key
(UnorderedKey)
[warning] 24-24: [LeadingCharacter] Invalid leading character detected
(LeadingCharacter)
[warning] 24-24: [UnorderedKey] The key should go before the <<<<<<< HEAD key
(UnorderedKey)
[warning] 31-31: [ExtraBlankLine] Extra blank line detected
(ExtraBlankLine)
[warning] 32-32: [IncorrectDelimiter] The >>>>>>> c45745f (Security: Make scoring a hard requirement by default) key has incorrect delimiter
(IncorrectDelimiter)
[warning] 32-32: [KeyWithoutValue] The >>>>>>> c45745f (Security: Make scoring a hard requirement by default) key should be with a value or have an equal sign
(KeyWithoutValue)
[warning] 32-32: [LeadingCharacter] Invalid leading character detected
(LeadingCharacter)
[warning] 32-32: [LowercaseKey] The >>>>>>> c45745f (Security: Make scoring a hard requirement by default) key should be in uppercase
(LowercaseKey)
🤖 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/.env.example` around lines 18 - 32, Resolve the merge conflict in agent/.env.example by removing the conflict markers and keeping the intended example settings from both sides. Preserve the service selection entries such as AGENT_MIN_SERVICE_REPUTATION and AGENT_MAX_SERVICE_RETRIES, and include the new AGENT_REQUIRE_SCORING documentation/value from the security change so the dotenv example remains valid and complete.Source: Linters/SAST tools
agent/agent.js (2)
196-211: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Resolve the committed merge-conflict markers.
<<<<<<<,=======, and>>>>>>>are present in executable source, so this module will not parse and the agent cannot start.Suggested direction
Keep one implementation for each conflicted region and remove all conflict markers. Re-run the parser/linter afterward to catch any follow-on scope/signature errors.
-<<<<<<< HEAD ... -======= ... ->>>>>>> 7b9eb5e (Fix agent scoring/runTask flow, restore agent summary coverage, and add docstrings)Also applies to: 314-326, 340-456
🧰 Tools
🪛 Biome (2.5.0)
[error] 196-196: Expected a statement but instead found '<<<<<<< HEAD'.
(parse)
[error] 201-201: Expected a statement but instead found '======='.
(parse)
[error] 211-211: Expected a statement but instead found '>>>>>>> 7b9eb5e (Fix agent scoring/runTask flow, restore agent summary coverage, and add docstrings)'.
(parse)
[error] 211-211: numbers cannot be followed by identifiers directly after
(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 196 - 211, Resolve the merge conflict in agent/agent.js by removing all conflict markers (`<<<<<<<`, `=======`, `>>>>>>>`) and keeping the correct implementation for each conflicted block. Make sure the `buildHttpClient` function and the `dispose` export both remain properly defined and in valid scope, then verify the rest of the conflicted regions in the module are similarly cleaned up so the file parses again.Source: Linters/SAST tools
498-500: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Remove the stale
httpClientargument fromrunTask().The new
runTask(category, buildUrl, scoringEnabled)builds its own client; passinghttpClienthere can reference an undefined binding after the conflict is resolved.Proposed fix
- const result = await runTask(category, buildUrl, scoringEnabled, httpClient); + const result = await runTask(category, buildUrl, scoringEnabled);📝 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.for (const { category, buildUrl } of tasks) { const result = await runTask(category, buildUrl, scoringEnabled); if (result.success) {🤖 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 498 - 500, The runTask invocation is still passing a stale httpClient argument even though runTask(category, buildUrl, scoringEnabled) now creates its own client; update the call site in the task loop to pass only category, buildUrl, and scoringEnabled. Use the runTask function and the surrounding tasks iteration in agent.js to locate and remove the extra argument so the code no longer depends on an undefined binding.
|
@Samaro1 please resolve conflicts |
- Add AGENT_REQUIRE_SCORING env var (default true) - Exit with error when registration fails and AGENT_REQUIRE_SCORING=true - Log warning when AGENT_REQUIRE_SCORING=false - Update ensureRegistered to use logger.error for all failure modes - Add post-registration verification step - Document AGENT_REQUIRE_SCORING in .env.example - Update tests for new behavior
Samaro1
force-pushed
the
security/require-scoring-hard-failure
branch
from
4cdbc9a to
5cb67f3
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/.env.example`:
- Around line 18-32: Resolve the merge conflict in the .env example by removing
all literal conflict markers and choosing the intended final configuration. Keep
the `AGENT_REQUIRE_SCORING` block from the security change and remove the
obsolete `AGENT_MIN_SERVICE_REPUTATION`/`AGENT_MAX_SERVICE_RETRIES` section
unless those settings are still meant to be documented. Make sure the
`agent/.env.example` file is valid plain env content with no `<<<<<<<`,
`=======`, or `>>>>>>>` lines.
In `@agent/agent.js`:
- Around line 111-120: The registration verification log in agent.js is
hard-coding the starting score as 100 even though verifyAgent.score is used to
populate currentScore. Update the logger.info call in the post-registration
verification block to report the actual verified score value (the same one
assigned from verifyAgent.score) so the message stays consistent with the
backend response and the currentScore state.
- Around line 400-438: The payment request flow in runTask() is bypassing the
x402 client path by signing a bare url/method payload and calling fetch()
directly. Update the payment logic to use the x402 client flow with
createPaymentPayload() and the client’s fetch handling so the 402 challenge is
resolved correctly before treating the request as paid. Keep the existing
failure handling in runTask(), but ensure it only runs after the x402-backed
request flow has been attempted using client and paymentPayload.
🪄 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: 3aba6d35-a70e-4cac-952d-44961641968b
📒 Files selected for processing (3)
agent/.env.exampleagent/agent.jsagent/agent.test.js
🚧 Files skipped from review as they are similar to previous changes (1)
- agent/agent.test.js
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
🤖 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/.env.example`:
- Around line 18-32: Resolve the merge conflict in the .env example by removing
all literal conflict markers and choosing the intended final configuration. Keep
the `AGENT_REQUIRE_SCORING` block from the security change and remove the
obsolete `AGENT_MIN_SERVICE_REPUTATION`/`AGENT_MAX_SERVICE_RETRIES` section
unless those settings are still meant to be documented. Make sure the
`agent/.env.example` file is valid plain env content with no `<<<<<<<`,
`=======`, or `>>>>>>>` lines.
In `@agent/agent.js`:
- Around line 111-120: The registration verification log in agent.js is
hard-coding the starting score as 100 even though verifyAgent.score is used to
populate currentScore. Update the logger.info call in the post-registration
verification block to report the actual verified score value (the same one
assigned from verifyAgent.score) so the message stays consistent with the
backend response and the currentScore state.
- Around line 400-438: The payment request flow in runTask() is bypassing the
x402 client path by signing a bare url/method payload and calling fetch()
directly. Update the payment logic to use the x402 client flow with
createPaymentPayload() and the client’s fetch handling so the 402 challenge is
resolved correctly before treating the request as paid. Keep the existing
failure handling in runTask(), but ensure it only runs after the x402-backed
request flow has been attempted using client and paymentPayload.
🪄 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: 3aba6d35-a70e-4cac-952d-44961641968b
📒 Files selected for processing (3)
agent/.env.exampleagent/agent.jsagent/agent.test.js
🚧 Files skipped from review as they are similar to previous changes (1)
- agent/agent.test.js
🛑 Comments failed to post (3)
agent/.env.example (1)
18-32: 🎯 Functional Correctness | 🔴 Critical
Resolve the merge conflict before merging.
This file still contains literal conflict markers, so the example env file is invalid as-is and the intended configuration is ambiguous. Keep only the final
AGENT_REQUIRE_SCORING=trueblock if that is the desired replacement.Suggested cleanup
-<<<<<<< HEAD -# Service selection hardening -# Minimum reputation a service must have to be eligible for payment (default: 0) -AGENT_MIN_SERVICE_REPUTATION=0 -# Max candidates to consider (weighted random) and max retry attempts per task (default: 3) -AGENT_MAX_SERVICE_RETRIES=3 -======= # Require scoring/spending policy enforcement (default: true) # When true, the agent will exit with error if registration fails, preventing # unprotected agent runs without spending limits. Set to false only for # development/testing where you explicitly want to run without policy enforcement. AGENT_REQUIRE_SCORING=true - - ->>>>>>> c45745f (Security: Make scoring a hard requirement by default)📝 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.# Require scoring/spending policy enforcement (default: true) # When true, the agent will exit with error if registration fails, preventing # unprotected agent runs without spending limits. Set to false only for # development/testing where you explicitly want to run without policy enforcement. AGENT_REQUIRE_SCORING=true🧰 Tools
🪛 dotenv-linter (4.0.0)
[warning] 18-18: [KeyWithoutValue] The <<<<<<< HEAD key should be with a value or have an equal sign
(KeyWithoutValue)
[warning] 18-18: [LeadingCharacter] Invalid leading character detected
(LeadingCharacter)
[warning] 23-23: [UnorderedKey] The AGENT_MAX_SERVICE_RETRIES key should go before the AGENT_MIN_SERVICE_REPUTATION key
(UnorderedKey)
[warning] 24-24: [LeadingCharacter] Invalid leading character detected
(LeadingCharacter)
[warning] 24-24: [UnorderedKey] The key should go before the <<<<<<< HEAD key
(UnorderedKey)
[warning] 31-31: [ExtraBlankLine] Extra blank line detected
(ExtraBlankLine)
[warning] 32-32: [IncorrectDelimiter] The >>>>>>> c45745f (Security: Make scoring a hard requirement by default) key has incorrect delimiter
(IncorrectDelimiter)
[warning] 32-32: [KeyWithoutValue] The >>>>>>> c45745f (Security: Make scoring a hard requirement by default) key should be with a value or have an equal sign
(KeyWithoutValue)
[warning] 32-32: [LeadingCharacter] Invalid leading character detected
(LeadingCharacter)
[warning] 32-32: [LowercaseKey] The >>>>>>> c45745f (Security: Make scoring a hard requirement by default) key should be in uppercase
(LowercaseKey)
🤖 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/.env.example` around lines 18 - 32, Resolve the merge conflict in the .env example by removing all literal conflict markers and choosing the intended final configuration. Keep the `AGENT_REQUIRE_SCORING` block from the security change and remove the obsolete `AGENT_MIN_SERVICE_REPUTATION`/`AGENT_MAX_SERVICE_RETRIES` section unless those settings are still meant to be documented. Make sure the `agent/.env.example` file is valid plain env content with no `<<<<<<<`, `=======`, or `>>>>>>>` lines.Source: Linters/SAST tools
agent/agent.js (2)
111-120: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Log the verified score instead of hard-coding
100.Line 119 can contradict Line 116 when the backend returns a non-default score.
Proposed fix
- 'Registered and verified — starting score: 100' + `Registered and verified — starting score: ${currentScore}`📝 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.// Post-registration verification: confirm on-chain registration is visible const verifyRes = await fetch(`${LODESTAR_API_URL}/api/agents/${AGENT_ADDRESS}`); if (verifyRes.ok) { const verifyData = await verifyRes.json(); const verifyAgent = verifyData.agent ?? verifyData; currentScore = verifyAgent.score ?? 100; logger.info( { event: EVENT.AGENT_REGISTERED, agentAddress: AGENT_ADDRESS, score: currentScore, scoringEnabled: true }, `Registered and verified — starting score: ${currentScore}` );🤖 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 111 - 120, The registration verification log in agent.js is hard-coding the starting score as 100 even though verifyAgent.score is used to populate currentScore. Update the logger.info call in the post-registration verification block to report the actual verified score value (the same one assigned from verifyAgent.score) so the message stays consistent with the backend response and the currentScore state.
400-438: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # Verify x402 payment flow usage and update affected mocks/tests. rg -n -C3 'encodePaymentSignatureHeader|client\.fetch|fetch\(endpointUrl' agent/agent.js rg -n -C3 'mockHttpClient|encodePaymentSignatureHeader|fetch:' agent/agent.test.jsRepository: Stellar-Ecosystem/lodestar
Length of output: 5494
🏁 Script executed:
#!/bin/bash set -euo pipefail # Inspect the relevant sections in agent/agent.js and the matching tests. sed -n '210,260p' agent/agent.js printf '\n---\n' sed -n '380,455p' agent/agent.js printf '\n---\n' sed -n '1,340p' agent/agent.test.jsRepository: Stellar-Ecosystem/lodestar
Length of output: 16973
🏁 Script executed:
#!/bin/bash set -euo pipefail # Inspect runTask signature and any x402/payment-related usage elsewhere. sed -n '1,220p' agent/agent.js printf '\n---\n' rg -n 'x402|payment|required|encodePaymentSignatureHeader|createPaymentPayload|getPaymentRequiredResponse|submitReputation|recordOutcome' -S agent . --glob '!node_modules'Repository: Stellar-Ecosystem/lodestar
Length of output: 50382
🏁 Script executed:
#!/bin/bash set -euo pipefail # Inspect the failure-path tests near the reputation handling. sed -n '450,525p' agent/agent.test.jsRepository: Stellar-Ecosystem/lodestar
Length of output: 3417
🌐 Web query:
x402HTTPClient encodePaymentSignatureHeader createPaymentPayload fetch removed 402 challenge semantics💡 Result:
The x402 payment protocol utilizes the
x402HTTPClientto bridge HTTP requests with underlying payment scheme logic [1][2][3][4]. While standard implementations, such as thewrapFetchWithPaymentutility, continue to support automated handling of the 402 Payment Required status code [5][6][7][8], the specific methods mentioned—encodePaymentSignatureHeaderandcreatePaymentPayload—remain core components of thex402HTTPClientinterface [1][2][3]. There is no evidence thatx402HTTPClienthas "removed" 402 challenge semantics. Rather, the protocol has evolved toward standardized v2 flows that utilize header-based authentication [1][8][9]. Key clarifications on the usage of these methods include: - createPaymentPayload: This method is used by the client to generate a signed payment authorization after parsing the server's 402 requirements (often found in the PAYMENT-REQUIRED header or response body) [2][5][10][9]. - encodePaymentSignatureHeader: This method takes the generated payload and encodes it into the appropriate protocol header (e.g., PAYMENT-SIGNATURE for v2 or X-PAYMENT for v1) to be included in the retried request [1][2][8][9]. - Evolution of Semantics: Modern implementations often use interceptors or wrapper functions that manage these steps automatically [5][6][8]. Recent updates (e.g., commit bed69f3) have introduced hooks likeonPaymentRequiredto provide more granular, developer-defined control over how 402 challenges are handled before full payment processing occurs [11]. If you are encountering issues with these methods, ensure your implementation is aligned with v2 protocol specifications, which typically require the full payment requirement object to be passed during payload creation to avoid common signing errors [10].Citations:
- 1: https://github.com/coinbase/x402/blob/6a99ba02/go/http/client.go
- 2: https://docs.skale.space/get-started/agentic-builders/start-with-x402
- 3: https://docs.x402.org/getting-started/quickstart-for-buyers
- 4: https://docs.t402.io/reference/core
- 5: https://docs.t402.io/reference/client-libraries
- 6: https://docs.payai.network/x402/clients/typescript/fetch
- 7: https://cdn.jsdelivr.net/npm/thirdweb@5.116.1/src/x402/fetchWithPayment.ts
- 8: https://registry.npmjs.org/x402-axios
- 9: YojinHQ/jintel-py#7
- 10: https://dev.to/tufti/i-spent-3-sessions-debugging-x402-payment-signing-heres-the-shortcut-3d62
- 11: fffilimonov/x402@bed69f3
Use the x402 client fetch flow here.
runTask()signs a bare{ url, method }payload and callsfetch()directly, which skips the 402 challenge /createPaymentPayload()path used by the x402 client. Real services can reject that header, and the code will still record a payment failure and negative reputation vote.Proposed direction
- const paymentPayload = { url: endpointUrl, method: 'GET' }; - const paymentHeaders = client.encodePaymentSignatureHeader(paymentPayload); let response; try { - response = await fetch(endpointUrl, { headers: paymentHeaders, keepalive: true }); + response = await client.fetch(endpointUrl, { method: 'GET', keepalive: true });📝 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.let response; try { response = await client.fetch(endpointUrl, { method: 'GET', keepalive: true }); } catch (err) { logger.error( { event: EVENT.PAYMENT_FAILED, category, serviceId: selected.id, serviceName: selected.name, priceUsdc: selected.price_usdc, err, taskDurationMs: Date.now() - taskStart, }, 'Payment failed — network error' ); if (scoringEnabled) await recordOutcome(selected.price_usdc, false, selected.id); failed.add(selected.id); continue; } if (!response.ok) { logger.error( { event: EVENT.PAYMENT_FAILED, category, serviceId: selected.id, serviceName: selected.name, priceUsdc: selected.price_usdc, httpStatus: response.status, taskDurationMs: Date.now() - taskStart, }, 'Payment failed — endpoint error' ); if (scoringEnabled) await recordOutcome(selected.price_usdc, false, selected.id); // Payment settled but service returned bad data — penalise service reputation. await submitReputation(selected.id, false);🤖 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 400 - 438, The payment request flow in runTask() is bypassing the x402 client path by signing a bare url/method payload and calling fetch() directly. Update the payment logic to use the x402 client flow with createPaymentPayload() and the client’s fetch handling so the 402 challenge is resolved correctly before treating the request as paid. Keep the existing failure handling in runTask(), but ensure it only runs after the x402-backed request flow has been attempted using client and paymentPayload.
|
@Samaro1 please fix ci |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/ci.yml (1)
127-136: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winAgent job still references a lockfile the fix removed elsewhere.
Lines 131-132 keep
cache: npm/cache-dependency-path: agent/package-lock.jsonforactions/setup-node, while backend/frontend had their equivalent cache config removed per the commit description. Ifagent/package-lock.jsondoesn't exist (consistent with.gitignoreexcludingpackage-lock.jsonrepo-wide), this cache config is inconsistent with the same fix applied elsewhere and should likely be removed too.Proposed fix
- name: Setup Node.js uses: actions/setup-node@v4 with: node-version: '22' - cache: npm - cache-dependency-path: agent/package-lock.json🤖 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 @.github/workflows/ci.yml around lines 127 - 136, The Agent job still configures npm caching against a lockfile that appears to be removed elsewhere, so update the Setup Node.js step to match the other jobs by removing the cache-related settings. In the actions/setup-node@v4 block, drop cache and cache-dependency-path from the agent workflow so it no longer references agent/package-lock.json, keeping the Agent job consistent with the backend/frontend fix.
🤖 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 @.github/workflows/ci.yml:
- Line 82: The CI workflow jobs currently use npm install, which can resolve
dependencies differently on each run and conflicts with the documented npm ci
workflow. Update the three job steps in the CI workflow back to npm ci so
installs are lockfile-pinned, and make sure the backend, frontend, and agent
jobs use the same reproducible command. If the intent is to keep npm install,
then align CONTRIBUTING.md and workspace lockfile handling accordingly, but the
preferred fix is to restore npm ci in the affected steps.
---
Outside diff comments:
In @.github/workflows/ci.yml:
- Around line 127-136: The Agent job still configures npm caching against a
lockfile that appears to be removed elsewhere, so update the Setup Node.js step
to match the other jobs by removing the cache-related settings. In the
actions/setup-node@v4 block, drop cache and cache-dependency-path from the agent
workflow so it no longer references agent/package-lock.json, keeping the Agent
job consistent with the backend/frontend fix.
🪄 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: 08dd478b-5cf9-4ad8-a8bd-08f39df1bffd
📒 Files selected for processing (1)
.github/workflows/ci.yml
| - name: Install dependencies | ||
| working-directory: backend | ||
| run: npm ci | ||
| run: npm install |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
npm install weakens CI reproducibility and contradicts CONTRIBUTING.md.
Switching from npm ci to npm install across all three jobs allows dependency versions to drift between CI runs (semver ranges resolved at install time), unlike npm ci which enforces the lockfile exactly. This also now conflicts with CONTRIBUTING.md, which still documents npm ci && npm test for backend, frontend, and agent. If lockfiles are intentionally gitignored, consider instead committing package-lock.json for each workspace and reverting to npm ci, or update CONTRIBUTING.md to reflect the new npm install workflow.
Also applies to: 104-104, 136-136
🤖 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 @.github/workflows/ci.yml at line 82, The CI workflow jobs currently use npm
install, which can resolve dependencies differently on each run and conflicts
with the documented npm ci workflow. Update the three job steps in the CI
workflow back to npm ci so installs are lockfile-pinned, and make sure the
backend, frontend, and agent jobs use the same reproducible command. If the
intent is to keep npm install, then align CONTRIBUTING.md and workspace lockfile
handling accordingly, but the preferred fix is to restore npm ci in the affected
steps.
2 participants
Closes #134
Fixes a security vulnerability where agents could run without spending limits when registration fails.
Changes:
Add AGENT_REQUIRE_SCORING environment variable (default: true)
When AGENT_REQUIRE_SCORING=true and registration fails, the agent now exits with error instead of continuing unprotected
When AGENT_REQUIRE_SCORING=false, logs a prominent warning for development/testing scenarios
Update ensureRegistered() to use logger.error for all failure modes (503, network unreachable, registration call failed, verification failed)
Add post-registration verification step to confirm on-chain registration is visible
Document AGENT_REQUIRE_SCORING in .env.example
Update tests for new behavior
Summary by CodeRabbit
AGENT_REQUIRE_SCORING(enabled by default) to enforce spending/scoring policy, including clearer startup messaging when enforcement can’t be activated.