Repository navigation
fix: unify runtime policies and repair audited modules - #711
Conversation
Share unlimited-by-default tool-call policy and correct built-in prompt facts. Repair independently confirmed runtime, credential, sharing, API and client defects with bounded module audit records and regression evidence. Refs #710
![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: 8f6491e30a
ℹ️ 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".
| parts: [{ id: wakeIdentity.partID, type: "text", text: summary, synthetic: true }], | ||
| }, | ||
| store.markWakeBatchReported(batch), | ||
| { continueToolBudget: true }, |
There was a problem hiding this comment.
When a DAG child batch spawned by input A completes after a later manual input B has been claimed, continueToolBudget only suppresses creation of a budget; it does not carry A's budget or execution-attempt identity. claimSnapshot therefore falls back to the session-global state.active, now B's, so the stale A wake can spend B's remaining calls—allowing A's continuation to exceed its configured cap—or be incorrectly blocked when B exhausted its budget. Pass the originating input/budget identity through wake admission instead of reusing whichever budget is active at delivery time.
AGENTS.md reference: AGENTS.md:L24-L24
Useful? React with 👍 / 👎.
| !methods.has(input[0].toLowerCase()) || | ||
| !input[1].startsWith("/") || | ||
| input[1].startsWith("//") || | ||
| input[1].includes("\\") |
There was a problem hiding this comment.
When a raw API request includes a Windows path as query data, such as /file?path=C:\repo\a.ts, this blanket check rejects the request before URL resolution. Backslashes after ? remain query data and cannot change the daemon origin, while /file explicitly accepts a string path query, so previously valid Windows calls now fail unless callers manually percent-encode the value. Restrict the backslash check to the authority/path portion or rely on the subsequent origin validation.
Useful? React with 👍 / 👎.
| // Forward only protocol negotiation. Authentication belongs to the selected provider. | ||
| export function providerRequestHeaders(incoming: Headers) { | ||
| const headers = new Headers() | ||
| for (const name of ["content-type", "accept", "anthropic-version", "anthropic-beta", "openai-beta"]) { |
There was a problem hiding this comment.
For Zen BYOK requests that select a non-default OpenAI organization or project, this allowlist drops OpenAI-Organization and OpenAI-Project before openaiHelper replaces only the authorization header. The supported OpenAI provider configuration emits both routing headers, and the Zen provider record stores only the API key, so affected requests can fail authorization or be attributed to the wrong project. Allow these non-secret routing headers while continuing to strip caller credentials and cookies.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7f5d39e6e1
ℹ️ 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".
| .update(KeyTable) | ||
| .set({ timeDeleted: sql`now()` }) | ||
| .where(and(eq(KeyTable.userID, id), eq(KeyTable.workspaceID, Actor.workspace()))) |
There was a problem hiding this comment.
When an admin removes and later re-invites an existing account, this update tombstones all of the member's keys, but User.invite restores the soft-deleted user and its lookup at lines 113-119 treats the tombstoned key as an existing key because it does not filter timeDeleted. It therefore skips Key.create, leaving the restored member without an active key accepted by the Zen authentication queries. Re-invitation should create a replacement key or explicitly restore an appropriate key.
Useful? React with 👍 / 👎.
| when={sync().ready || !sync().error} | ||
| fallback={ | ||
| <HomeBootstrapError | ||
| error={formatServerError(sync().error, language.t, language.t("common.requestFailed"))} |
There was a problem hiding this comment.
When the user selects a secondary server whose bootstrap fails while the route's primary server is healthy, the page data comes from focusedSync() but this error boundary still reads sync(). The secondary failure is therefore hidden behind an empty/stale home view, and the displayed retry action can target the primary server instead of the selected failing server. Read the error, retry state, and retry callback from the focused server context.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0591558cb7
ℹ️ 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".
| const current = await state(input.share.id) | ||
| if (!current || current.value.deleted) throw new Errors.NotFound(input.share.id) | ||
| if (current.value.secret !== input.share.secret) throw new Errors.InvalidSecret(input.share.id) | ||
| const data = current.value.version === 2 ? (current.value.data ?? []) : await legacy(input.share.id) |
There was a problem hiding this comment.
During an Enterprise deployment with an old request still in flight, that request can acknowledge a sync by writing share_snapshot after a new worker has read the legacy objects. Because the legacy write does not change the share ETag, the new worker's CAS can still promote stale data to version 2, after which this branch permanently bypasses the acknowledged snapshot update. Fence or merge legacy writes during migration, or enforce a deployment compatibility gate.
AGENTS.md reference: AGENTS.md:L26-L26
Useful? React with 👍 / 👎.
| if (!options?.continueToolBudget) { | ||
| const state = yield* budgetState(input.sessionID) | ||
| state.pending.set(message.info.id, ToolBudget.create((yield* config.get()).maxToolCalls)) |
There was a problem hiding this comment.
With finite maxToolCalls, this fresh-budget default also applies to the automatic structured-result continuations in dag/runtime/spawn.ts and dag/runtime/recovery.ts, whose prompt() calls omit continueToolBudget. A node can therefore exhaust its original allowance, encounter a stale structured result, receive the runtime resubmission nudge, and spend a new allowance within the same node execution attempt. Pass the continuation option at both nudge sites, as is already done for the missing-result continuation.
AGENTS.md reference: AGENTS.md:L24-L24
Useful? React with 👍 / 👎.
| get ready() { | ||
| return !bootstrap.isPending | ||
| }, | ||
| ready: false, |
There was a problem hiding this comment.
When a server is removed and the same URL is re-added before its shared [scope, "bootstrap"] query is garbage-collected, refetchOnMount: false lets the recreated context consume the cached successful query without executing its queryFn. This new store nevertheless starts with ready: false, and only that skipped function sets it to true, so the legacy home remains on its loading state and readiness-gated synchronization never runs. Initialize readiness from the cached query state or remove the bootstrap query when disposing the server context.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d2c1ce3cb0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return messages | ||
| // Activate only inputs included in this snapshot. Capture the budget | ||
| // under the same lock so later admissions cannot replace its owner. | ||
| const budget = latestPending?.budget ?? state.active ?? ToolBudget.create((yield* config.get()).maxToolCalls) |
There was a problem hiding this comment.
When a finite-budget turn is resumed after the daemon restarts or its directory scope is disposed, this fallback cannot recover the consumed count because both pending and active budgets existed only in InstanceState memory. DAG recovery and manual loop/resume can then continue the same persisted input with a fresh allowance, so restarting after exhaustion bypasses maxToolCalls and can repeat side effects; persist or reconstruct the input's consumed budget rather than creating it anew here.
AGENTS.md reference: AGENTS.md:L24-L24
Useful? React with 👍 / 👎.
| $schema: Schema.optional(Schema.String).annotate({ | ||
| description: "JSON schema reference for configuration validation", | ||
| }), | ||
| maxToolCalls: Schema.optional(ToolBudget.MaxToolCalls), |
There was a problem hiding this comment.
Adding maxToolCalls to the public ConfigV1 schema changes both GET/PATCH /config, but only the v2 generated type was updated. The package's default @opencode-ai/sdk export still uses src/gen/types.gen.ts, whose Config type backs config.update requests and config get/update responses and omits this field, so typed clients cannot configure the new limit and do not see it in returned configuration; regenerate or update the default SDK contract and its checked-in OpenAPI artifact alongside this schema.
AGENTS.md reference: AGENTS.md:L26-L26
Useful? React with 👍 / 👎.
| if ( | ||
| !(await Storage.compareAndSwap( | ||
| ["share", body.id], | ||
| { version: 2, deleted: true, revision: crypto.randomUUID() }, |
There was a problem hiding this comment.
During a rolling deployment, this retained object is truthy to the previous version's get/data path, so an old sync that passed its secret check before removal can recreate share_snapshot after the cleanup and an old reader will then serve the deleted share again. Encode deletion so legacy readers still observe absence, or prevent mixed-version access, to preserve revocation across upgrades.
AGENTS.md reference: AGENTS.md:L26-L26
Useful? React with 👍 / 👎.
1 participant
Issue for this PR
Closes #710
Why
The audit independently confirmed inconsistent tool-call policies, acknowledged data loss and credential-boundary defects. Repair those findings and record separate confirmations before the requested stable release.
Type of change
What changed
Unify tool-call budgets and repair 16 independently confirmed issues from the requested module audit.
maxToolCallspolicy, default0(unlimited). Count actual tool attempts. Automatic continuation keeps the input budget; manual Goal new/resume gets a fresh budget. Correct built-in prompt facts and share AFK timeout guidance.What does this PR do?
The audit record and adjacent evidence cover all 36 workspace packages, the independent VS Code SDK, Go and infrastructure. Coverage follows entry points, critical actions and error/lifecycle paths; it does not prove every function or branch correct. All 222 protected original working-tree files retain their starting hashes.
CLI publication does not deploy Enterprise or Console cloud services. Enterprise conditional writes require coordinated replacement of old unconditional writers.
no-storecannot remove existing cached or downloaded shares. Actual Electron/VS Code host interaction was not exercised.Evidence
Each repaired issue has independent second confirmation. Astra approved the final integrated code and both OpenAPI review corrections, with no remaining blockers.
How did you verify your code works?
Focused regressions use independent reproductions and isolated actual runtime or HTTP boundaries. The broad package pipeline and browser checks passed. Merge requires the four protected native checks and the triggered four-platform Nix verification on this final head. Stable publication then uses the existing main-only release workflow with all platforms, followed by tag/Latest/artifact/SHA256SUMS readback.
Screenshots / recordings
Synthetic initial bootstrap failure, followed by a working Retry control. Both layouts are exercised by the Chromium recovery regression.
Checklist
Native CI test isolation repair
CI-001: Linux CI exposed process-wide Stripe module mocks polluting the real inference regression. Root and sol independently reproduced the same failure in fixed test order. The inference fixture now runs in a bounded, cleaned-up child process with the same Bun executable and all 10 original assertions. The fixed-order probe passed 29 tests; the complete Console App package passed 36 tests, with typecheck and zero-warning scoped lint. Astra supplemental final review approved this repair; final-head native checks remain required before merge. This test-isolation repair does not add a duplicate initial product finding.
Native CI timing repair
CI-002: the pathological schema test previously mixed subprocess startup, compatibility validation, pathological validation and process shutdown in one 3-second watchdog. Independent synthetic startup-delay probes confirmed this accounting problem; the old native failure has no phase evidence, so its actual delayed phase remains unknown. The repair adds finite startup admission and phase diagnostics while retaining the production 250 ms budget, validation <=1000 ms, heartbeat >=5 and original ready-to-exit 3-second deadline. Focused 15 tests, typecheck and the full final DAG gate passed. Astra supplemental final review approved this repair. Final-head native checks remain required before merge.
Native CI Goal wiring isolation repair
CI-003: retaining a real test InstanceStore scope in the shared memoMap caused AppRuntime to reuse its empty bootstrap implementation. Root and sol independently reproduced the original 8-second failure with zero Goal init/subscription calls; releasing that scope restored production initialization. The original native scope holder was not recorded and remains unknown. Both production probes now execute unchanged in a bounded fresh Bun process, with the original 8-second poll and 20-second per-test limits. Root confirmed the wrapper passes while the parent still retains the polluted cache. Goal regressions, typecheck and scoped lint passed. Astra supplemental final review approved the repair with no blockers and independently ran the wrapper. Final-head native checks remain required before merge.
App test fixture and environment repairs
CI-004: native App unit tests exposed the submit test’s process-wide SDK factory mock in the bootstrap fixture. Root and luna independently reproduced both constructor failures. A file-local SDK fixture preserves the real bootstrapGlobal call, all three tests and fourteen assertions. The fixed-order probe now passes eight tests and twenty-six assertions. Full App unit and browser tests and typecheck pass.
CI-005: additional randomized-order acceptance exposed DOM tests resolving Solid’s server web renderer. Root and luna independently reproduced seed 2 with default exports and confirmed the same full suite passes with browser exports. Unit and watch commands now select browser conditions, matching the existing browser test script. Production code, dependencies and the original assertions are unchanged. This is a test-environment repair, not a demonstrated historical native CI trigger. Astra approved both completed repairs with no blockers. The fixture is partial and its method signatures are not established by its type assertion. Final-head native gates remain required.