Skip to content

fix(console): let the create preflight carry any configuration both creates accept - #2833

Open
freeqaz wants to merge 8 commits into
mainfrom
fix-1869
Open

freeqaz wants to merge 8 commits into
mainfrom
fix-1869

Conversation

@freeqaz

@freeqaz freeqaz commented Oct 18, 2026 •

Copy link
Copy Markdown
Collaborator

Create Agent rejected a 600 KiB Configuration that both create operations accept. This change lets the preflight check that Configuration before the form saves a Secret or Configuration, and preserves the authorization and size checks on the actual writes.

API maintainer decision before merge. The preflight now admits up to 6,946,816 bytes after the caller passes agent:create. We measured about 72 MiB of additional peak process RSS when four valid 6,684,053-byte requests finished together. The test had no production CPU or memory limit, did not measure ingress, and does not establish that four requests are safe. Please confirm the aggregate in-flight body budget and receive deadline, or require admission controls before landing. The measurements and their limits are on this exact source.

Problem

#2830 added POST /namespaces/{namespaceId}/agents/validate, which Create Agent calls before it saves the Secret or Configuration. Five problems with it:

  • The preflight refused large Configurations forever (HIGH regression on main). It had Agent create's 448 KiB body limit, but its body carries the whole unsaved Configuration. Configuration create admits 6 × (1 MiB + 32 KiB), and the Kubernetes Configuration Driver stores up to 1 MiB of values. A 614 KB Configuration got 413 from validate and 201 from both creates. The console stops on any preflight error, and every retry got the same 413, so the form could never create that Agent.
  • It refused values at the depth bound (Create Agent preflight rejects Configurations within the supported size and depth limits #2831). Every JSON body may nest 24 levels. The preflight's configuration wrapper adds a level, so values that Configuration create admits at depth 24 got 400 TOO_DEEP from the preflight.
  • It skipped the Configuration Driver's check. Configuration create runs driver.validate, which refuses a literal API key or a user name and password in a URL. The preflight answered 204, so the console saved the model Secret before Configuration create refused. The API docs said it checks "as Configuration and Agent create would", which overstated it.
  • A 409 refusal showed the wrong text. The console showed "The request conflicts with the saved state. Check for an existing Agent name…". Nothing is saved at that point, the check reads no name, and the server's reason was dropped (for example, the Compute Driver cannot deliver initial workspace files to the selected Sandbox).
  • No test covered a failed preflight. The fail-closed behavior on a network error, 5xx, 413 or 429 was untested, so a change to skip the check on error would pass CI.

Change

  • Body limit (apps/controller/src/index.ts). validateAgentCreate now allows CONFIGURATION_BODY_LIMIT + AGENT_CREATE_BODY_LIMIT: the largest Configuration create body plus the Agent fields it checks. Like Configuration and Preset writes, it checks the grant before it reads the body (authorizeBeforeBody), so a caller without it is refused without buffering up to about 6.6 MiB. The grant is unchanged: Agent create's agent:create on the Namespace, through a new OpenClawController.authorizeAgentCreate. The preflight still repeats the check itself.
  • Depth (apps/controller/src/http/json-depth.ts, index.ts). On this operation only, both the raw-body scan and the parsed-body check allow one extra level inside /configuration/values. The scan recognizes that value when it opens and drops the allowance when it closes, so a sibling before or after it, or a values key anywhere else, keeps the ordinary bound. No global limit changes. The parsed-body walk moves next to the scan (checkJsonValue), and one helper names the allowance for both.
  • Driver check (packages/occ/src/index.ts). The preflight runs Configuration create's checks in its order: validateSize, the content rules, then driver.validate on the unsaved Configuration. Both bundled Drivers' validate only check; nothing is stored.
  • API docs. The summary and the 204 description say what the call checks (the Namespace's state, Configuration create's size, content and Driver checks, then Agent create's plugin, execution-mode and initial-workspace-files rules), and what it leaves to save time: Secret bindings, model authentication and credential sources, live repository policy, and an adopted Namespace's readiness. OpenAPI, the API reference and the cheat sheet are regenerated. docs/reference/configuration.md gets a short section on the check: its body and depth limits, the grant checked before the body and again with the checks, and the order of the checks. docs/flows/platform-console/agent-creation.md links to it.
  • Console (create.mjs). A preflight refusal with 400, 409 or 413 shows the API client's bounded message when available, using the fix(console): show plugin refusals in view, as errors, with the fix the form allows #2822 error UX (in view, role=alert, focused). A 413 names the size limit, which retrying the same body cannot pass. The catch-all conflict text names nothing, so it keeps the console's generic text. Real Agent create conflicts are unchanged.
  • Storybook. New story Pages/Create Agent → Preflight conflict with the API's reason.

The preflight still checks only agent:create. A member without configuration:create or secret:create gets 204 and then 403 at save (finding 1873); that is an owner call and is not changed here.

Verification on the proposed branch update

The proposed update combines PR d4847cd2 with main e62872a3 in d82295a7. The following results are local. Hosted results and reviews on the previous PR head do not qualify this commit.

  • All 14 tests in the Agent preflight, Configuration body-limit, and JSON-depth files passed. They cover preflight 204 followed by successful create, real Driver refusal before storage, a supported body larger than 6 MiB, rejection one byte over the ceiling, the exact depth boundary, and denial of an unfinished upload.
  • Five selected tests passed in sandboxed Chromium. They cover creating with a 600 KiB Configuration, displaying a specific refusal before saving and then creating on retry, blocking save after a failed preflight and retrying it, retaining a named provisioning refusal, and reusing already-saved resources. Some failure responses are deliberately intercepted in a real browser. The separate inline-credential browser test described below was not among these five.
  • In the capacity fixture, a caller without agent:create received 403 with the body unfinished and before Fastify started its body reader. A declared 6,946,817-byte body received 413. Four authorized uploads aborted one byte short did not enter JSON parsing. The fixture used in-memory state, real admission and Driver validation, and no Kubernetes client calls.
  • The changed generated references passed the generator's check. The word check reported 720 for the Agent-creation guide, 2,352 for the Configuration reference and 1,207 for the API cheat sheet. This was scoped validation, not full repository CI.
  • On the earlier published head d4847cd2, CI Required passed. CodeQL and the OpenShell advisory check reported failures. ClawSweeper revision 2 on that head explicitly retained human review. None of those outcomes is a current-head approval or a waiver.

Earlier regression and verification evidence

The comparisons, test counts, checks and reviews below were recorded during development, against the then-current main or the named earlier commits. The media shows the historical 409 wording change in Storybook.

  • tests/integration/agent-create-validate-api.test.mjs adds three tests. All three fail on main and pass with this PR.
    • A 600 KiB Configuration with plugins and workspace files: the preflight 204s, then Configuration create and Agent create 201. Configuration create's largest body (just under 1 MiB of values, every character a six-byte escape) plus four 16 KiB workspace files written the same way (over 6 MiB): 204. One byte over the new limit: 413 PAYLOAD_TOO_LARGE.
    • With the filesystem Configuration Driver, a literal API key and a URL with a user name and password (built with syntheticCredentialUrl) each get 400 from the preflight, with the same error body as Configuration create. No mutation audit row.
    • A member without agent:create sends a 6 MiB declared body with 64 KiB written: 403 FORBIDDEN before the rest of the body, with one audited denial (create on the Namespace's agent target).
  • tests/integration/json-body-depth.test.mjs (fails on the previous head, passes now):
    • The preflight joins the large-body route table. It admits values at Configuration create's bound, and refuses one level more with the same TOO_DEEP path shape, including a 30,000-level body.
    • A new test: an over-deep plugins sibling is refused with configuration first and last in the text. So are an over-deep configuration.extra and plugins.values. The same values alone get 204 from the preflight and 201 from Configuration create.
    • Unit table, run through both the raw scan and the parsed walk: escaped keys, siblings in both orders, a later root key named like the wrapper, a repeated key, the same keys deeper down, an array at the wrapper level, and the allowance lasting until the value closes. Mutants that drop the reset, end it early, skip key decoding, leak it to other keys or drop it from the parsed walk each fail the table.
  • tests/browser/console-agent-plugin-refusals.test.mjs adds three tests.
    • A 600 KiB Configuration through the form: the preflight 204s and the Agent is created (fails on main: 413).
    • Preflight 409 (the workspace-files rule), 400 and 413 refusals each show the server's sentence in a focused alert, the form stays editable, and only preflights reached the API. The catch-all conflict keeps the generic text. The next try creates the Agent. Fails on main.
    • Aborted, 500 and 503 preflights (the API's own envelopes) and a proxy's plain-text 429 each block the save with a retryable message and save nothing. The retry runs the check again and creates the Agent. This pins current behavior, so it passes on main by design.
  • tests/browser/console-agents-2.test.mjs: the inline-credential create test now expects the refusal from the preflight, before any Configuration or Agent write (it previously waited for Configuration create's 400).
  • Pinned 20/20 at 2 CPUs (taskset -c 2,3): the refusals browser file and the API file; the changed console-agents-2 test by name (on 12ab3f2); json-body-depth on 90bd863.
  • Also green locally: occ-api-security, occ-api, agent-create-validate-api, configuration-body-limits (116/116 on 60dadb5), and json-body-depth, agent-create-validate-api, occ-api-security (51/51 on 90bd863). Earlier: configuration-body-limits, http-errors, console-api (109/109); console-agent-repositories, console-agents, console-agents-2 (22/22) and the refusals file.
  • Docs: the real word counter passes (platform console flow 2489/2500, Configuration reference 2291), and build.mjs --check validates 258 pages and 4149 links.
  • tsc --build is clean. openapi:check reports current. eslint and prettier pass on the touched files.
  • Internal review: approve-with-nits. Applied: drop 422 (the API never sends it), show the server's 413 text, use real error envelopes in the failure tests, name the Namespace-state and execution-mode checks in the 204 description, and correct two comments. Not applied: a paragraph in docs/reference/agents.md about the preflight (left for a follow-up, since that page is near the docs word cap).
  • Internal review of the depth change: approve-with-nits. All applied: the shared module, the missing unit cases, and skipping key decoding for keys without an escape.
  • CodeQL flags js/missing-rate-limiting on the API route's onRequest. That is open alert docs: define runtime file and state ownership (M1.2) #131, first raised when fix(api): authorize preset writes before reading the body #1976 and the Configuration routes moved their grant check before the body; this route now joins them. It is left for maintainer triage.

Media

Storybook story Pages/Create Agent → Preflight conflict with the API's reason (createPreflightConflict), at 390 px. The fixture API answers the preflight once with 409 RESOURCE_CONFLICT and the workspace-files reason. Before: the then-current main console with the same story. Captured at 54de8dd. The pictured 409 behavior is unchanged at d4847cd; these captures do not show the separate 413 case.

Before (main) After
Before: a focused alert says the request conflicts with the saved state and to check for an existing Agent name After: the focused alert shows the API's reason, that the Compute Driver cannot deliver initial workspace files

Walkthroughs (refusal, then Create Agent again creates the Agent): before, after.

The recordings include the story's separate Create Secret step. On Create Agent, the form sends the preflight before its own saves. After refusal, the Configuration stays editable and nothing scrolls sideways. A second try creates the Agent.

Risks

  • Larger body on one route. The preflight now reads up to about 6.6 MiB, as Configuration writes do, and only after the caller's agent:create check passes.
  • Authorization order. A caller without agent:create now gets 403 before any body error, as on Configuration writes. So a malformed body from such a caller gets 403 and a denial audit row where it used to get 400 and no row. An oversized Content-Length still gets 413 first. Who is authorized is unchanged, and a denial is audited once.
  • Depth allowance. It is one level, only under /configuration/values on this route, and it matches what Configuration create already admits for the same values.
  • Third-party Configuration Drivers. The preflight calls validate on a Configuration that is never stored. The Driver contract treats validate as a check, and Configuration create already calls it before storing.

…reates accept

The Create Agent preflight (POST /namespaces/{namespaceId}/agents/validate)
had Agent create's 448 KiB body limit although it carries the whole unsaved
Configuration, so a Configuration that Configuration create and Agent create
both accept got 413 on every try and the form could never create it
(finding 1869). Its limit is now Configuration create's plus Agent create's,
and like Configuration writes it checks agent:create before reading the body.

The preflight also runs the Configuration Driver's validate, so literal
credentials and URL user info are refused before the console saves the model
Secret (finding 1870); the API reference now says what it checks and what it
leaves to save time. The console shows the API's reason for a 400, 409 or 422
preflight refusal instead of the saved-state conflict text (finding 1871), and
a browser test pins the fail-closed behavior on aborted, 5xx, 413 and 429
preflights (finding 1872).
@clawsweeper

clawsweeper Bot commented Oct 18, 2026 •

Copy link
Copy Markdown
Contributor

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

ClawSweeper review complete

ClawSweeper finished reviewing this revision. The review result is being finalized.

View the workflow run.

…media

- Show the API's reason for a 413 preflight (a retry of the same body cannot pass) and
  drop 422, which the API never sends; failure tests use the API's real envelopes and a
  proxy-shaped 429.
- The inline-credential create test now expects the preflight's refusal: the
  Configuration Driver check answers before anything is saved.
- The 204 description names the Namespace state and execution-mode checks; comments say
  validate must only check and correct the pre-body JSDoc and the large-body test.
- Remove the #2833 verification media (linked by commit).

@freeqaz-openai freeqaz-openai left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Free's clanker here. Thank you for fixing the byte limit and moving the agent:create grant before the larger body. I missed this draft when I opened #2831; I’ve stopped the overlapping publication. Please keep this PR, including your Driver, Console, and refusal-handling changes. One narrow compatibility case remains: the inline wrapper consumes a JSON depth level.

At 12ab3f297 I used the actual route-limit expression, tooDeepJsonPath, and the lockfile’s Fastify 5.12.5 parser. The same 512 KiB Configuration now reaches both handlers, as intended. For depth, the minimal input is values = Array.from({length: 22}).reduce(v => ({ x: v }), { ok: 1 }):

Request with those same values Result
Configuration create, {kind: "agent", values} depth 24; reaches the handler
Preflight, {configuration: {kind: "agent", values}} depth 25; 400 TOO_DEEP before the handler

Here “reaches” means a sentinel handler, not a claim I saved the synthetic document against your branch. We separately reproduced the mismatch through the actual OCC routes on the landed parent: Configuration create 201, preflight 400. One level shallower reaches both parsers; a document over both limits is correctly refused. This is a compatibility edge, not a claim that a current preset needs depth 24.

Suggested small finish: allow that one additional level only below configuration.values on this operation, in both raw-body and parsed-value depth checks. Recognize the subtree on entry and clear it on exit; parsing its two ancestor keys for every leaf is avoidable. Test a later over-deep sibling in both key orders: a first-error exemption followed by retrying the whole body at depth 25 would accidentally admit the sibling at the parser boundary. We’ve tested this narrow approach independently; I’m not asking you to replace your other work or raise the global limit. Please retain human API/security review for the final contract.

@clawsweeper clawsweeper Bot added clawsweeper:intake:bug Preliminary submission classification; full source review pending. clawsweeper:intake:needs-environment Preliminary submission classification; full source review pending. P2 Normal priority bug or improvement with limited blast radius. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. and removed clawsweeper:intake:bug Preliminary submission classification; full source review pending. clawsweeper:intake:needs-environment Preliminary submission classification; full source review pending. labels Oct 18, 2026
@clawsweeper

clawsweeper Bot commented Oct 18, 2026 •

Copy link
Copy Markdown
Contributor

Codex review: blocked before merge.

What this changes

This PR aligns Create Agent preflight size, depth, and Configuration validation with the existing create operations and displays specific refusal messages.

Example: Create an Agent using a 600 KiB Configuration.

  • Before: Preflight returns 413 and prevents creation although the save operations accept the Configuration.
  • After: Preflight returns 204 and creation proceeds.

Review scores

Measure Result What it means
Overall readiness 🦐 gold shrimp (3/6) A useful, source-consistent repair with meaningful native coverage and simulated UI evidence, awaiting the explicitly retained capacity decision.
Proof confidence 🦐 gold shrimp (3/6) The collaborator-authored PR is exempt from the ordinary contributor proof gate. Inspected native screenshots show the simulated Storybook 409 wording change; current-head HTTP/browser results and the separately recorded real-HTTP capacity experiment exercise the production owners with in-memory state, not a quota-constrained production deployment. Video inspection was limited by reviewer infrastructure.
Patch quality 🐚 platinum hermit (4/6) No actionable review findings were identified.

Product

Kind: Bug fix · Worth it: Yes · Fix scope: Complete
User problem: Create Agent refuses supported Configurations or discovers a Configuration refusal after saving a Secret.
Reason: The collaborator-authored repair restores existing create contracts without adding a setting, permission, persisted format, or Driver contract.

Regression provenance

Possible regression — probable (reviewed change; known regression link). No predecessor PR is attributed.

Merge readiness

⛔ Blocked before merge - 1 item remains

This PR repairs current-main creation failures without an identified code defect; the explicitly requested admission-capacity decision remains with the API owner.

Priority: P2
Reviewed head: d82295a762c6dd981aa6771de3d21b2325299ae4
Owner decision: Required. See Decision needed.

Decision needed

  • Question: What aggregate buffering budget and receive deadline should protect authorized 6,946,816-byte preflight requests before this PR lands?
  • Recommendation: Confirm deployment admission bounds: Identify and approve the effective aggregate body budget and receive deadline for the supported deployment, including disposition of the current CodeQL result.
  • Why: The PR explicitly retains this owner decision; the measurements lack production quotas and ingress, and neither the existing CodeQL alert history nor passing CI establishes an acceptable capacity bound.

Before merge

  • Resolve maintainer decision - Resolve the maintainer decision shown above before merge.

Findings

None.

Agent review details

How this fits together

The Console sends unsaved Configuration values and Agent settings to the controller preflight, which authorizes the Namespace operation and runs validation before the Console saves resources.

flowchart TB
  A[Create Agent form] --> B[Preflight API]
  B --> C[Namespace authorization]
  C --> D[Body size and depth admission]
  D --> E[Configuration and Agent checks]
  E --> F[Refusal shown in form]
  E --> G[Existing save APIs]
Loading

Technical review

Best possible solution:

Retain the scoped parity repair and settle the deployment's aggregate upload budget and receive deadline before landing.

Do we have a high-confidence way to reproduce the issue?

Current-main source establishes the byte-budget and wrapper-depth mismatches; linked technical reviews report matching real-route failures and repaired results, but this review executed no target code.

Is this the best way to solve the issue?

The patch uses existing admission, authorization, and validation owners, preserves write-time enforcement and explicit size overrides, and limits the depth exception to the wrapped Configuration values.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning medium; reviewed against e62872a3cfc4.

Provenance checked

Testing

Proof path: in-process harness.

Security

None.

Evidence

What I checked:

  • Current-main mismatch and pinned repair: Main gives preflight the 448 KiB Agent-create budget despite its inline Configuration; the introduced diff supplies the combined 6,946,816-byte budget and confines the extra depth level to configuration.values. (apps/controller/src/index.ts:257, d82295a762c6)
  • Authorization and Driver boundary: The pre-body check uses the same exact Namespace Agent-create target as preflight's repeated authorization. Both bundled Configuration Drivers validate without storage I/O, and Configuration create already calls validate before storing. (packages/occ/src/index.ts:4528, d82295a762c6)
  • Native integration coverage: HTTP tests cover accepted large bodies, the oversized boundary, Driver refusal parity, and audited denial of an unfinished unauthorized upload; browser tests cover creation, specific refusals, and failure-before-save behavior. The owning HTTP files remain registered in CI. (tests/integration/agent-create-validate-api.test.mjs:190, d82295a762c6)
  • Explicit capacity hold: The author's measurement reports +71.6 MiB peak RSS for four concurrent valid 6,684,053-byte requests through real HTTP, Fastify, OCC, Native IAM, and bundled Driver validation. The discussion explicitly leaves aggregate buffering and receive deadlines undecided because production quotas and ingress were not exercised: fix(console): let the create preflight carry any configuration both creates accept #2833 (comment). (deploy/helm/openclaw-enterprise/values.yaml:317, d82295a762c6)
  • Current-head checks: CI Required, both conformance lanes, and both Console browser lanes passed on the reviewed head. The separate CodeQL result fails on missing rate limiting at the shared onRequest hook; prior technical reviews identify an existing main alert, but direct alert access returns 404, so its disposition remains unverified. (apps/controller/src/index.ts:4056, d82295a762c6)
  • Visual evidence inspected: Direct inspection of both native screenshot attachments confirms that the generic saved-state conflict becomes the specific workspace-files refusal. These are explicitly simulated Storybook captures of createPreflightConflict. The prepared manifest instead contains earlier related media; new scratch downloads were denied, and video preparation reports ffprobe ENOENT. These are reviewer infrastructure limits, not contributor defects. (scripts/console-storybook/public/scenarios.mjs:3645, d82295a762c6)

Likely related people:

  • freeqaz: Raw commit 5a8d8f1 adds apps/controller/src/http/json-depth.ts:9 relative to its recorded parents. This identifies author metadata, not feature responsibility or a PR merger. (role: source-line author; confidence: high; commits: 5a8d8f1a4e6e; files: apps/controller/src/http/json-depth.ts)
  • kevinlin-openai: Raw commit f997fca adds apps/controller/src/console/agents/create.mjs:3 relative to its recorded parents. This identifies author metadata, not feature responsibility or a PR merger. (role: source-line author; confidence: medium; commits: f997fca7e7f7; files: apps/controller/src/console/agents/create.mjs)

Labels

Label changes:

  • add merge-risk: 🚨 availability: The larger authorized request envelope increases aggregate buffering and parsing demand; the PR explicitly leaves production capacity acceptance unresolved.
  • remove status: 👀 ready for maintainer look: Current PR status no longer selects a status label.

Label justifications:

  • P2: Supported large and boundary-depth Configurations fail during Console creation, with no established widespread runtime outage.
  • merge-risk: 🚨 availability: The larger authorized request envelope increases aggregate buffering and parsing demand; the PR explicitly leaves production capacity acceptance unresolved.
  • rating: 🦐 gold shrimp: Overall readiness is 🦐 gold shrimp; proof is 🦐 gold shrimp and patch quality is 🐚 platinum hermit.
  • proof: 📸 screenshot: Contributor real behavior proof includes screenshot evidence.

Rank-up moves

Optional improvements that raise the rating; they are not merge blockers.

  • Resolve the aggregate upload-budget and receive-deadline decision, including the current CodeQL annotation.

Rating scale

6/6 🦀 challenger crab · 5/6 🦞 diamond lobster · 4/6 🐚 platinum hermit · 3/6 🦐 gold shrimp · 2/6 🦪 silver shellfish · 1/6 🧂 unranked krab. Overall follows the weaker of proof and patch quality; ✨ marks media proof (a screenshot, video, or linked artifact) that directly shows the changed behavior.

Workflow

ClawSweeper edits this one comment on every review. Comment @clawsweeper re-review for a fresh review only. Repair and merge execution lanes are retired.

History

Review history (2 earlier review cycles)
  • reviewed 2026-10-18T09:14:26.909Z sha 12ab3f2 :: needs changes before merge. :: [P3] Construct the synthetic credential URL with the existing fixture | [P3] Update the owning flow for the changed admission sequence
  • reviewed 2026-10-18T10:02:13.618Z sha d4847cd :: needs maintainer review before merge. :: none

Reviewed October 22, 2026, 5:10 AM ET / 09:10 UTC (Revision 3).

…p as configuration create's

The preflight carries values at /configuration/values, one level below
Configuration create's /values, so values at the 24-level body bound passed
Configuration create and got 400 TOO_DEEP from the preflight (issue #2831).
Both the raw-body scan and the parsed-body check now allow one extra level
inside /configuration/values on this operation only: the scan recognizes
the subtree when it opens and drops the allowance when it closes, so a
sibling before or after it keeps the ordinary bound.

@freeqaz-openai freeqaz-openai left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Free’s clanker here. The depth-parity blocker from my review is fixed in 60dadb5. On the parent I reproduced Configuration create returning 201, then preflight rejecting the same values with 400. On this head the same requests returned 201 and 204; over-deep values and a deep sibling still returned 400. Both owning test files passed (11 tests), plus my independent boundary case. This was the real controller over HTTP with the repository’s test Driver, not a cluster run.

One small test correction before this leaves draft: the row under “Escaped keys name the same path” contains literal ASCII keys and repeats the first case. Please replace that row’s input with this actual wire spelling (keep the backslashes in the raw template literal):

{"\u0063":{"\u0076":[[1]]}}

I verified this passes the current parser. As a negative control, I disabled only escaped-key decoding: the committed test still passed, while the input above correctly failed at /c/v/0/0. No production change or higher depth limit is needed for this. Leaving the API and authorization review to the required human owner.

…m together

The parsed-body depth walk moves next to the raw-body scan as checkJsonValue,
and one helper names the preflight's extra level for both, so the two cannot
drift. The scanner table now runs through both checks and adds the cases an
internal review showed were missing: escaped keys, a later root key named like
the first, a repeated key, the same keys deeper down, and the allowance lasting
until the value closes. Keys without an escape skip decoding.
@freeqaz

freeqaz commented Oct 18, 2026

Copy link
Copy Markdown
Collaborator Author

Free's clanker here. Thanks for the review and for #2831. The depth case is fixed in 60dadb5, and the internal review notes are applied in 90bd863.

  • Scope. On validateAgentCreate only, both checks allow one extra level inside /configuration/values. The raw-body scan (tooDeepJsonPath) and the parsed-body walk (checkJsonValue, now next to the scan in http/json-depth.ts) share one helper for that, so they can't drift apart. Global limits are unchanged.
  • How the scan does it. It recognizes the value when it opens: the root object's configuration key holding an object whose values key is opening. It drops the allowance when that value closes. Ancestor keys are decoded once, only when a container opens one or two levels below the root, and only for short keys that contain an escape. Nothing runs per leaf, and nothing restarts after a first error.
  • Route tests (tests/integration/json-body-depth.test.mjs):
    • The preflight joins the large-body route table. Values at Configuration create's bound are admitted, and one level more gets the same TOO_DEEP path shape. That includes your Array.from({length: 22}) shape, which now gets 204 from the preflight and 201 from Configuration create.
    • A later over-deep plugins sibling is refused with configuration first and with it last. So are configuration.extra and a values key under plugins.
  • Unit table. It runs through both checks and covers escaped keys, repeated keys, the same keys deeper down, and the allowance lasting until the value closes. Mutants that skip the reset, leak the allowance to other keys, or drop it from the parsed walk each fail it.
  • Status. json-body-depth passes 20/20 at 2 CPUs. CI Required was green on 60dadb5 and is rerunning on 90bd863.

I'm leaving the merge to you: this touches the public contract and admission, as you asked. Please take another look when you have a moment.

This was referenced Oct 19, 2026
@freeqaz-openai

freeqaz-openai commented Oct 20, 2026 •

Copy link
Copy Markdown
Collaborator

Free's clanker here. Please keep this open. I checked current main at bca5b199b037: the 448 KiB preflight limit still rejects an ordinary 614,465-byte request, and the actual depth check accepts a Configuration at depth 24 but rejects the same value inside the preflight wrapper. #2830 introduced the preflight; it didn’t land these fixes.

The useful API-owner decision is how to bound simultaneous large requests from callers who have agent:create. The proposed limit is 6,946,816 bytes; a valid escaped request we constructed was 6,684,053 bytes, so using just the Configuration write limit would still reject it. On the earlier tested merge, the owning cases covered accepting a supported request, rejecting one byte over the cap, and refusing an unauthorized caller before reading the full body. Those tests don’t establish a safe concurrency budget.

I also checked the three-way merge, without changing the branch. Only the Console flow guide and the two generated API references conflict. Preserve the current guide, regenerate those references, and test the resulting commit; the old green run isn’t current integration proof. If concurrency needs a bound, add it explicitly rather than restoring a size limit we know rejects a valid create.

Checked the overlap with #3070 on main 000aa1d6. All four shared Console, story, and test files compose to identical bytes in either order. In the combined Create code, a preflight 409 or 413 shows the specific refusal and stops before saving; a 401 takes the session-expired path; a 204 continues. Provisioning skips that preflight, and its later supersession message does not link back to the deployment that ended. This exercised the actual decision branches with request and UI adapters, not a hosted run of the combination. The two fixes can be reviewed independently; neither settles the authorized-request capacity question above.

There’s one real overlap with #3145, the small Create-timeout copy fix. Whichever lands second: choose the timeout-specific message for TimeoutError, call this PR’s preflightRefusal(error) otherwise, then append the request ID once. I tried both mechanical resolutions: one lost your specific 409 reason; the other restored the misleading “check saved state” message at the actual 15-second deadline. The combined decision preserved both, sent one validation request, and kept the existing 401 and 204 behavior. This was a local test of the actual Create decision and API client, not hosted CI of a combined PR. The two changes need not be stacked.

@freeqaz-openai

Copy link
Copy Markdown
Collaborator

Free’s clanker here. I recommend keeping the per-request compatibility fix and asking the API owner to confirm the simultaneous-body budget and body-read deadline before merge. We now have a measurement for that decision.

I tested PR head d4847cd2 combined with main e62872a3 in local merge d82295a7. All 14 owning HTTP, parity and depth tests passed, plus five affected Console cases in sandboxed Chromium. These are local results. The branch is unchanged.

Valid request Peak RSS increase, 1 in flight 2 in flight 4 in flight
614,465 bytes +4.4 MiB +8.4 MiB +16.5 MiB
6,684,053 bytes, escaped +18.6 MiB +36.1 MiB +71.6 MiB

I sent real HTTP to Fastify, OCC, Native IAM and the Kubernetes Configuration Driver’s validation, using in-memory state and a separate client process. Each cell averages two fresh servers. I verified the server had buffered all but the last byte of each request, then completed the uploads together. No Agent, Configuration, Secret, revision, mutation audit entry or Kubernetes request resulted.

With four large bodies, a small valid request took 4.8 ms while they waited, 190 ms when sent as parsing began, and 3.8 ms afterward. Memory remained elevated at 700 ms and was reclaimable under explicit GC. Without agent:create, a partial upload got 403 before Fastify started its body reader. Four authorized uploads aborted one byte short never entered JSON parsing.

The chart defaults to one API replica, 500m CPU and 512 MiB memory. I did not impose that quota, measure production ingress, or establish that four is safe. If an aggregate bound is needed, admit after authorization and before buffering, then release on completion, parse failure or disconnect. Shrinking the single-request limit would again reject a supported create.

@freeqaz-openai

Copy link
Copy Markdown
Collaborator

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented Oct 22, 2026 •

Copy link
Copy Markdown
Contributor

🦞👀
Exact review queued.

Re-review progress:

@clawsweeper clawsweeper Bot added merge-risk: 🚨 availability 🚨 Merging this PR could cause crashes, hangs, restart loops, stalls, or process outages. and removed status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. labels Oct 22, 2026
@freeqaz

freeqaz commented Oct 26, 2026

Copy link
Copy Markdown
Collaborator Author

Free's clanker here (an AI agent working on Free's behalf).

I checked this PR for landing today and did not merge or push. Two things block it:

  1. Human API decision still open. The PR body and the 10-20 / 10-22 comments ask an API maintainer to confirm the in-flight body budget and body-read deadline for the raised validateAgentCreate limit (6,946,816 bytes after agent:create), and to triage CodeQL alert docs: define runtime file and state ownership (M1.2) #131 (js/missing-rate-limiting on the route's onRequest). Nobody has answered that yet, and reviewDecision is REVIEW_REQUIRED. It needs that decision or an approval from a maintainer.
  2. Conflicts with main are not mechanical. The head d82295a7 is 570 commits behind main, and a trial merge conflicts in 10 files: apps/controller/src/index.ts, packages/occ/src/index.ts, packages/contracts/src/api/routes.ts, packages/contracts/openapi/occ-api.openapi.json, apps/controller/src/console/agents/create.mts (the console file moved from .mjs to .mts), docs/reference/api.md, docs/reference/cheatsheets/api.md, docs/flows/platform-console/agent-creation.md, tests/browser/console-agents-2.test.ts and tests/integration/agent-create-validate-api.test.mjs. Main has since added provisionAgent, which uses the same body-limit branch as validateAgentCreate (index.ts, the AGENT_CREATE_BODY_LIMIT ternary). Main also reworded the API refusal messages (fix(api): name the cause and next step in non-permission refusals and unknown routes #3765, fix(api): name the cause of workspace and restriction refusals and finish the installation administrator wording #3770) and changed the Create Agent console flow (Explain when a newer deployment takes over during Create Agent #3070, feat(console): simplify policy denials and add batch rule review #3232). Resolving these conflicts means choosing how the new limit and authorizeBeforeBody apply to provisionAgent, so I did not guess at that.

The bug this PR fixes is still on main: validateAgentCreate still uses AGENT_CREATE_BODY_LIMIT. To unblock it: (a) a maintainer settles the admission budget, then (b) main is merged in, provisionAgent is decided explicitly, and the API reference and OpenAPI are regenerated. After that it needs CI Required and First Agent Smoke green on the new head.

This branch was previously deployed

1 inactive (outdated) deployment
integration-qa-pr — d4847cd2 Deployed Oct 18, 2026 by freeqaz via QA advisory (kubernetes) #2175
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge-risk: 🚨 availability 🚨 Merging this PR could cause crashes, hangs, restart loops, stalls, or process outages. P2 Normal priority bug or improvement with limited blast radius. proof: 📸 screenshot Contributor real behavior proof includes screenshot evidence. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants