feat(integrations): run catalog tools with local credentials - #3843
Conversation
📦 Client bundle boundary
A server module in a client graph aborts hydration in the browser. New leaks fail CI; known leaks are tracked in |
|
Warning Review limit reached
Next review available in: 21 minutes Limit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits within each organization. For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (2)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThis change adds account-free local integration tools. The implementation validates catalog grants, resolves project credentials, executes guarded provider REST requests, exposes typed errors, and documents local, Salesforce, self-hosted, and remote execution boundaries. ChangesLocal integration runtime
Estimated code review effort: 5 (Critical) | ~120 minutes Merge Risk: 🟡 Moderate · up to This PR adds local credential-backed catalog execution, but malformed credentials may cause opaque request failures, catalog validation may prevent unrelated integrations from importing, and part of the test suite may be skipped by Node and Bun; documentation also currently describes the capability inaccurately. Merge should wait for these bounded risks to be fixed or explicitly accepted. Sequence Diagram(s)sequenceDiagram
participant Agent
participant LocalToolSource
participant CredentialProvider
participant Provider
Agent->>LocalToolSource: load exact-grant tools
LocalToolSource->>CredentialProvider: resolve named credential
CredentialProvider-->>LocalToolSource: return credential value
LocalToolSource->>Provider: execute guarded authenticated request
Provider-->>LocalToolSource: return provider response
LocalToolSource-->>Agent: return transformed result
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d7eeae958b
ℹ️ 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".
|
Exact-head follow-up 1ec401f addresses every current blocker:
Verification: 173 integration steps GREEN, focused Deno 22 steps GREEN, focused Node 22 tests GREEN, typecheck GREEN, lint/docs GREEN, npm build GREEN, fmt/lint/diff checks GREEN. All current review threads are resolved. @codex review |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
git show --no-ext-diff --format=fuller --stat 1ec401fd3
printf '\n--- changed files ---\n'
git diff-tree --no-commit-id --name-status -r 1ec401fd3
printf '\n--- relevant diff ---\n'
git show --no-ext-diff --format= --find-renames --find-copies 1ec401fd3 -- \
| sed -n '1,1800p'
printf '\n--- candidate symbols ---\n'
rg -n -i -C 3 \
'ETRUSTED_CLIENT_(ID|SECRET)|audience|tokenParameters|token parameters|Trusted Shops|Headers\.set|Array\.push|String\.prototype|primordials' \
. \
-g '!node_modules/**' -g '!dist/**' -g '!build/**' \
| sed -n '1,1800p'Length of output: 201004 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '--- commit metadata and changed files ---'
git show --no-ext-diff --format='commit %H%nparents %P%nsubject %s' --name-status 1ec401fd3
printf '%s\n' '--- complete follow-up patch ---'
git show --no-ext-diff --format= --find-renames --find-copies 1ec401fd3 \
| sed -n '1,2200p'
printf '%s\n' '--- local credential auth outline ---'
ast-grep outline src/integrations/local-credential-auth.ts --items all --view expanded
printf '%s\n' '--- focused implementation and test evidence ---'
for file in \
src/integrations/local-credential-auth.ts \
src/integrations/local-credential-auth.test.ts \
src/integrations/local-endpoint-executor.test.ts \
src/integrations/_data.ts \
src/integrations/_data.test.ts
do
if [ -f "$file" ]; then
printf '\n### %s\n' "$file"
rg -n -C 8 \
'trusted-shops|ETRUSTED_CLIENT|additionalAuthParams|token-request|client_credentials|audience|ambient collection|framework-owned traversal|Headers\.set' \
"$file" || true
fi
doneLength of output: 71393
The update resolves the catalog credential-name mismatch for Trusted Shops. It also copies validated catalog token parameters into client-credentials requests and encodes The Node test changes avoid host-runtime and I found no new blocking issue in this follow-up. 🧠 Learnings used
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1ec401fd30
ℹ️ 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".
|
@codex review |
Record the explicit local source, credential-provider contract, supported authentication modes, egress controls, and fail-closed non-goals before implementation begins. Constraint: Local integration credentials must never enter model-visible, persisted, logged, URL, or Veryfront API state Rejected: Reuse integrations.allow as a grant surface | it is an established narrowing-only policy Rejected: Fall back implicitly from the hosted remote source | transport and credential ownership would become ambiguous Confidence: high Scope-risk: broad Directive: Keep the local source explicit and preserve the API-backed integration path unchanged Tested: Markdown format, line-width audit, placeholder audit, and diff whitespace Not-tested: Runtime behavior; implementation follows in RED-GREEN tasks
Translate the approved credential boundary into admission, authentication, endpoint execution, Salesforce, documentation, verification, and merge-gate tasks with explicit RED and GREEN evidence. Constraint: Each credential-boundary behavior must be locked by a failing regression before implementation Confidence: high Scope-risk: broad Directive: Do not merge until exact-head CI is green and all review threads are resolved Tested: Markdown format, line-width audit, placeholder audit, and diff whitespace Not-tested: Runtime behavior; plan execution starts next
Introduce an explicit catalog tool allowlist, snapshot it before project mutation, emit credential-free model schemas, and reject unsupported endpoint/auth contracts before any provider request exists. Constraint: Environment credential presence must never enable a tool by itself Rejected: Reuse catalog visibility as the local grant | explicit Salesforce service tools must work without widening public catalog/scaffolding visibility Confidence: high Scope-risk: moderate Directive: Keep source construction as the grant and integrations.allow as narrowing-only Tested: 5 focused Deno steps; deno check; deno fmt; deno lint; git diff --check Not-tested: Credential resolution and provider execution follow in later TDD seams
Separate serializable catalog auth plans from short-lived resolved headers and token requests, validate readiness before tool exposure, and support API-key, Basic, client-credentials, and Salesforce service-account vocabularies. Constraint: Credential providers receive only canonical environment-variable names and provider failures cannot carry secret causes Rejected: Store raw credentials on the tool source | source objects and metadata are model/runtime-visible for longer than one request Confidence: high Scope-risk: moderate Directive: Do not add raw credential values to plans, errors, contexts, caches, or logs Tested: 13 focused Deno steps; deno check; deno fmt; deno lint; git diff --check Not-tested: Provider HTTP transport and token-response handling follow in the endpoint execution seam
Catalog metadata now constructs exact-origin REST requests without exposing provider responses or transport failures. The executor rejects unknown or mistyped arguments before transport, applies defaults deterministically, rejects redirects, shares the guarded egress boundary, and bounds JSON response reads. Constraint: Local integration credentials and provider response bodies must never enter errors, URLs outside the admitted origin, or unbounded memory. Rejected: Reuse ordinary fetch directly | it would bypass redirect-hop and private-address enforcement. Confidence: high Scope-risk: moderate Directive: Keep transport injection internal and preserve exact-origin authorization on every network hop. Tested: 19 focused Deno steps; deno check; deno lint; deno fmt; anti-slop; diff and line-width checks. Not-tested: Live third-party provider responses and token endpoints remain for the next slice.
The catalog-backed source now resolves project credentials at call time, mints short-lived OAuth client credentials through the guarded transport, validates Salesforce My Domain instance origins, and executes only the admitted tool endpoint. Local materialization keeps the existing source-policy narrowing and leaves the hosted API bridge unchanged. Constraint: Salesforce service-account responses may supply an instance origin, but never an arbitrary provider URL or redirect target. Rejected: Cache access tokens in the source | it would lengthen secret lifetime and complicate rotation and invalidation. Confidence: high Scope-risk: moderate Directive: Keep the test transport seam internal; the public source accepts only exact tool grants and a name-only credential provider. Tested: 110 focused Deno steps covering local execution, source policy, and the unchanged remote API bridge; deno check, lint, fmt, anti-slop, diff and line-width checks. Not-tested: Live provider credentials and third-party availability.
The public guides now separate managed OAuth from the catalog-backed local credential path and lock the exact-grant and secret-isolation contracts with executable documentation tests. Constraint: Local execution must not require or contact the Veryfront control plane. Rejected: Document integrations.allow as credential selection | it is narrowing-only capability policy. Confidence: high Scope-risk: narrow Directive: Keep managed per-user OAuth distinct from local service-account execution. Tested: docs validation, guide contracts, executable examples, public copy checks, pinned Deno 2.7.7 API reference check Not-tested: live provider credentials
Capture the collection, URL, abort, response, and string primitives used by local integration admission and execution. Adversarial tests now prove project code cannot replace those ambient primitives to widen grants, erase credentials, or skip bounded response cleanup. Constraint: Project modules share the host realm before local sources list or execute tools. Rejected: Trust ambient collection prototypes | project code can replace them after framework initialization. Confidence: high Scope-risk: moderate Directive: Keep local integration admission and credential execution on captured primitives and own-property array writes. Tested: 15 integration suites (215 steps), focused hardening suites (28 steps), production check, fmt, lint, anti-slop, test-typecheck baseline, docs validation, public docs, error docs, pinned API-reference generation/check. Not-tested: Live provider credentials or production provider endpoints.
Review hardening captures credential-bearing catalog data before project code can mutate public exports, keeps credential identifiers out of model-visible definitions, reports only bounded provider diagnostics, and carries required Basic-auth headers. Constraint: Local credential values and credential identifiers must never enter model metadata or provider diagnostics Rejected: Re-read the mutable public connector catalog | project code can mutate it before source construction Confidence: high Scope-risk: narrow Directive: Keep local admission on the private catalog snapshot and preserve exact-grant semantics Tested: 15-suite integration and tool matrix (39 tests, 245 steps); docs validation; pinned API docs; npm build
Validate catalog credential metadata and tool arguments before credential resolution, treat successful no-content responses as null, register the local boundary errors centrally, and document the direct local execution boundary. Constraint: No provider request or credential lookup may occur for an invalid tool invocation Rejected: Rely only on endpoint execution validation | OAuth minting happens before endpoint execution Confidence: high Scope-risk: moderate Directive: Keep argument admission before all credential work and preserve 204 as a successful empty result Tested: 15-suite integration and tool matrix (38 tests, 243 steps); error registry (119 slugs); docs validation (1,385 links); deno check, lint, fmt, anti-slop, diff checks Not-tested: Live provider credentials and third-party endpoints
The local-integration guide assertions now resolve their files from the test module URL, preserving deterministic reads when parallel tests temporarily change the shared process directory. Constraint: Parallel test isolates share process cwd and the lint baseline cannot increase. Rejected: Raise the cwd-relative-read baseline | would preserve a known race instead of removing it. Confidence: high Scope-risk: narrow Directive: Resolve repository fixtures from import.meta.url in test callbacks. Tested: lint:cwd-relative-test-reads; guide-content.test.ts; fmt; lint; check; diff-check Not-tested: Full suite deferred to the existing pre-push hook and replacement CI.
49ecfe5 to
9ab5b17
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9ab5b172b9
ℹ️ 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".
|
Review follow-up at exact head 5fe6ca2:
Verification: focused Deno 40/40 steps, all integration tests 176/176 steps, targeted type-checking, repository lint/static gates, generated docs, npm package build, focused Node 24/24 tests, focused Bun 2/2 files, formatting, and diff checks are green. The full lint:ci run reached one unrelated floating-type failure in src/agent/runtime/provider-metadata-continuation.test.ts. It reproduces unchanged on the merged base with deno check --no-lock, while its locked check is green. Fresh exact-head CI is now authoritative for the PR. |
|
@codex review |
|
Codex Review: Didn't find any major issues. You're on a roll. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
Resolve doc and doc-contract conflicts by combining both sides: - self-hosting.md keeps the generic createLocalIntegrationToolSource example and restores main's Salesforce service-account section - salesforce.md keeps the local catalog example and main's dedicated service-account cross-reference - support-matrix keeps the split remote/local rows and notes the dedicated Salesforce service-account source - guide-contracts pins the union of both sides' surviving snippets - api-reference/integrations.md regenerated with pinned Deno 2.7.7
|
Merged current main at exact head
Verification: tests/docs 57 passed (240 steps); src/integrations 14 files (192 steps); typecheck, |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c26c7254a5
ℹ️ 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".
|
Both fixes are now on the branch at exact head The deferred hosted-gate finding is filed as veryfront/veryfront-issue-inbox#595 ( For the record on the earlier CI noise: head |
|
@codex please review the superseding exact head New in this head ( Please check specifically:
Still the substantive change from the prior head, please keep it in scope: the Exact-head GREEN: |
|
Answering my own question 3 above, since I traced it rather than leaving it open:
The catalog and the options object are supplied by the developer wiring the source up, not by the model, and those errors are thrown at configuration time before any tool call. Sanitizing them would actively hurt: collapsing a genuinely malformed catalog ID to So the rule this settles on is: sanitize where the value crosses the model boundary, keep it verbatim where it is developer configuration. Only Also worth noting the sanitized name is not the only signal in that message — the |
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
Regenerated with `deno task docs` on the CI-pinned Deno 2.7.7. The sanitization commit shifted three declarations in `local-tool-source.ts`; only the line pins move and no documented symbol changes. Refs veryfront/veryfront-issue-inbox#495
|
| symbol | pin |
|---|---|
createLocalIntegrationToolSource |
L581 -> L588 |
LocalIntegrationCredentialProvider |
L55 -> L58 |
LocalIntegrationToolSourceOptions |
L60 -> L63 |
No documented symbol was added, changed, or removed.
That also answers question 1 from my review request: exporting safeLocalIntegrationIdentifier did not widen the public API surface. It is absent from the regenerated reference because local-integration-errors.ts is not re-exported through src/integrations/index.ts; the module stays internal and the helper's only consumer in src/ is the call site it was added for. Verified by grep, not inferred from the docs alone.
Note the other 8 required checks were already green on a12cbe647 — ci (lint) was the only failure, and coverage shard 4/8 passed this time.
…redentials
The dot-segment guard ran only in `buildRequest`, which `executeTool` reaches
after `mintLocalCredentialAuth`. For a client-credentials tool such as
`paypal__get_invoice`, an invalid `{ invoiceId: ".." }` therefore completed an
OAuth token round trip -- sending the client credentials to the token endpoint --
before anything rejected it. The pre-auth argument snapshot only coerced path
values with `scalarString`, so it let the value through.
`snapshotLocalIntegrationEndpointArguments` now validates path arguments with
the same `pathSegment` helper the builder uses, so the rejection happens before
any credential is resolved or minted and before any transport call. Header
arguments keep their `scalarString` coercion; only path values carry
path-segment semantics.
The build-time guard stays. The two are deliberately redundant: the executor is
exported and callable directly, so it cannot rely on a caller having snapshotted
first.
Reported by Codex review on this PR.
Refs veryfront/veryfront-issue-inbox#495
Pre-auth ordering fix pushed:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 60303e2057
ℹ️ 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".
The same ordering defect the path guard just fixed also applied to header arguments. The pre-auth snapshot only coerced them with `scalarString`, while the rejection for a value the `Headers` setter refuses happened in `buildRequest` -- after `mintLocalCredentialAuth`. `mongodb-atlas__list_projects` is client-credentials and takes an `accept` header argument, so a value containing CRLF completed an OAuth token round trip before anything rejected it. 87 catalog tools expose a header argument on an OAuth connector, 4 of them on a client-credentials one. The snapshot now runs the argument through the same `setHeader` helper against a throwaway `Headers`, so the header name and value are validated by exactly the code that will later set them, with no second copy of the rules. The invariant this settles on: every argument class is fully validated before any credential is resolved or minted. Refs veryfront/veryfront-issue-inbox#495
The same ordering defect applied to header arguments — fixedI asked in my last comment whether any other argument class could still trigger credential minting before validation. I traced it rather than waiting for an answer, and yes, header arguments had exactly the same defect. Reachability, measured against the catalog
FixThe pre-auth snapshot now runs header arguments through the same if (field.in === "path") pathSegment(resolved.value);
else if (field.in === "header") {
setHeader(new HeadersConstructor(), field.headerName ?? name, scalarString(resolved.value));
}Validating through the same code that will later set the header means there is no second copy of the rules to drift — the header name is checked too, not just the value. Verification
The invariant this settles onEvery argument class is fully validated before any credential is resolved or minted. Query and body arguments were already covered by Worth noting for the record: this is the second defect of this exact shape, and the first was only caught because Codex checked the ordering rather than the guard itself. The guard being correct said nothing about where it ran. |
|
@codex review the exact current head Both argument classes that previously deferred validation to if (field.in === "path") pathSegment(resolved.value);
else if (field.in === "header") {
setHeader(new HeadersConstructor(), field.headerName ?? name, scalarString(resolved.value));
}Please check specifically:
Reachability that motivated it, measured against the catalog: 87 tools take a header argument on an OAuth connector, 4 on a client-credentials one. Exact-head GREEN: |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 76ee731173
ℹ️ 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".
…entials Validating each body field on its own proves nothing about the size of the body they add up to, because an omitted field still contributes its catalog default. Supplied arguments could therefore pass the pre-auth snapshot while the body built from them exceeded the 4 MiB JSON bound, leaving `serializeJson` in `buildRequest` to reject it -- after `mintLocalCredentialAuth`. `salesforce__create_case` is the reachable case: four supplied strings just under the 1 MiB per-string cap serialize to under the limit, and the `Status` and `Origin` defaults carry the assembled body over it. The credential provider and the token endpoint were both invoked before the rejection. The body assembly is extracted as `assembleBody` and called from both the snapshot and `buildRequest`, so the bound is checked against the body that will actually be sent rather than against a reconstruction that could drift from it. Reported by Codex review on this PR. Refs veryfront/veryfront-issue-inbox#495
|
@codex review the exact current head New in this head: your assembled-body finding is fixed. Body assembly is extracted as Please focus on:
Verification at this head: Note: one pre-push run on this commit failed on the unrelated |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 85cad0e25c
ℹ️ 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".
Sharing `assembleBody` between the pre-auth snapshot and `buildRequest` made both calls run the same code, but not in the same environment. Caller-supplied credential code runs between them, and it can install an `Object.prototype.toJSON`. `JSON.stringify` honours an inherited `toJSON` for the ordinary nested objects a snapshot contains -- a null-prototype outer record does not help -- so the second assembly could produce a completely different body from the one that was bounded. Measured, not theorised: with a provider that poisons the prototype while resolving the credential, a `brevo__create_contact` call sent the literal body `"POISONED"` in place of the contact payload. The bound check is bypassed entirely, and the value sent to the provider under the project's credential is whatever that code chooses. `snapshotLocalIntegrationEndpointArguments` now returns the serialized body alongside the arguments, and `executeTool` threads it into the executor, which sends it verbatim. Direct executor callers still assemble locally: they have no credential step for the environment to change across. Reported by Codex review on this PR. Refs veryfront/veryfront-issue-inbox#495
|
@codex review the exact current head New in this head: your finding is fixed by threading rather than by hardening serialization. now returns Please focus on:
I deliberately did not take your alternative ("make serialization immune to prototype changes"): immunity would require rebuilding every nested object with a null prototype, which is more code and regresses silently, whereas threading makes sending unchecked bytes structurally impossible. Push back if you disagree. Exact-head GREEN: Housekeeping note: the docs regeneration also dirtied |
|
Correction to my previous comment — three phrases were eaten by shell backtick substitution before posting. Reposting intact; please review against this one. @codex review the exact current head New in this head: your Please focus on:
I deliberately did not take your alternative ("make serialization immune to prototype changes"): immunity would require rebuilding every nested object with a null prototype, which is more code and regresses silently, whereas threading makes sending unchecked bytes structurally impossible. Push back if you disagree. Exact-head GREEN: Housekeeping note: the docs regeneration also dirtied |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: da9654cf6e
ℹ️ 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".
…mbled Threading the pre-auth body stopped the executor from *sending* a re-serialized body, but it still *performed* the second assembly and discarded the result. That is invisible when a poisoned `toJSON` returns a value and fatal when it throws: any endpoint with a nested or passthrough body still failed after the credential had been resolved. Argument validation is split from body serialization. `snapshotAndValidateArguments` does the argument work alone, and the public snapshot is that plus one assembly. The executor validates arguments, then assembles only when no pre-auth body was threaded in, so a caller that already serialized never serializes twice. The previous test could not see this, because its `toJSON` returned a string rather than throwing, so the redundant assembly succeeded and its result was dropped. The new test throws instead. Reported by Codex review on this PR. Refs veryfront/veryfront-issue-inbox#495
|
@codex review the exact current head New in this head: your "skip reassembling a body that was already snapshotted" finding is fixed by splitting, not by gating. const args = snapshotAndValidateArguments(options.endpoint, options.args);
const body = options.body ?? assembleBody(options.endpoint, args);Please focus on:
Exact-head GREEN: Context worth stating plainly: this is the seventh finding on this PR, and the last four form a chain in which each fix left a smaller version of the same hole — guard ran too late, then checked the wrong object, then the environment moved between check and use, then the redundant work still executed. If you see a fifth link in that chain, I would rather hear it now than merge. |
|
Codex Review: Didn't find any major issues. Keep it up! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
1 participant
Description
Add an explicit catalog-backed integration source for local and self-hosted projects. Callers grant exact canonical tool IDs, resolve project-owned credentials by name at execution time, and call supported fixed HTTPS REST endpoints without a Veryfront account or project token.
The boundary supports header API keys, HTTP Basic, OAuth 2.0 client credentials, and the Salesforce service-account specialization. Unsupported authentication, endpoint, response, runtime, and catalog contracts fail closed. Credential values stay out of tool metadata, arguments, URLs, logs, errors, caches, and Veryfront APIs. The existing managed remote integration path is unchanged.
Documentation now presents the local path as first-class and distinguishes it from managed OAuth and remote execution.
Related Issue(s)
Completes the BYO integration-credentials ask in https://github.com/veryfront/veryfront-issue-inbox/issues/495
Type of Change
Verification
Checklist
Summary by CodeRabbit
New Features
Documentation
Tests