fix(core): make indexed access explicit across remaining src (NUIA phase 3b)#104773
Conversation
openclaw-barnacle
Bot
added
app: web-ui
58956d4 to
1ac75ce
Compare
![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: 58956d4890
ℹ️ 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".
| ...params.target.agents, | ||
| defaults: { | ||
| ...params.target.agents?.defaults, | ||
| ...expectDefined(params.target.agents?.defaults, "agent defaults"), |
There was a problem hiding this comment.
When app-guided provider setup starts from a sparse/first-run config that has agents.list but no agents.defaults, provider patches from onboard helpers can add prepared.agents.defaults.models; this branch now throws on expectDefined(params.target.agents?.defaults) before the probe/persist path. The old spread created the defaults object, so this merge needs to keep treating the missing container as {}.
Useful? React with 👍 / 👎.
| } | ||
| config.models = { | ||
| ...config.models, | ||
| ...expectDefined(config.models, "configured models"), |
There was a problem hiding this comment.
For providers whose setup patch carries models.providers and a user's base config has no models block, findSelectedProviderConfigKey is truthy and this spread now throws instead of creating models. That aborts app-guided activation before testing the credential; keep the previous empty-object merge behavior while inserting the selected provider config.
Useful? React with 👍 / 👎.
|
Codex review: needs real behavior proof before merge. Reviewed July 11, 2026, 11:45 PM ET / July 12, 2026, 03:45 UTC. Summary Reproducibility: yes. Source inspection gives deterministic paths for a session with no in-range usage summary and for errorBackoffMs receiving an empty schedule; both now throw where the base behavior returned a supported fallback value. Review metrics: 1 noteworthy metric.
Stored data model Root-cause cluster Members:
Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything. Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Proof guidance:
Risk before merge
Maintainer options:
Next step before merge
Maintainer decision needed
Security Review findings
Review detailsBest possible solution: Restore the nullable session row and empty-schedule fallback contracts, audit similar assertions that precede fallbacks, add focused regression coverage, and attach redacted exact-head runtime proof before approval. Do we have a high-confidence way to reproduce the issue? Yes. Source inspection gives deterministic paths for a session with no in-range usage summary and for errorBackoffMs receiving an empty schedule; both now throw where the base behavior returned a supported fallback value. Is this the best way to solve the issue? No, not yet. The phased type-safety burn-down is appropriate, but legitimate null and fallback states must be modeled explicitly rather than converted to invariant failures. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against ddbec923d4ee. Label changesLabel justifications:
Evidence reviewedAcceptance criteria:
What I checked:
Likely related people:
What the crustacean ranks mean
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics. How this review workflow works
Review history (4 earlier review cycles)
|
clawsweeper
Bot
added
rating: 🦪 silver shellfish
1ac75ce to
bbe409f
Compare
openclaw-barnacle
Bot
added
the
channel: qa-channel
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bbe409f694
ℹ️ 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 currentRoleContent = asRoleContentMessage(current); | ||
| const next = messages[i + 1]; | ||
| const next = expectDefined(messages[i + 1], "messages entry at i + 1"); |
There was a problem hiding this comment.
When projectChatDisplayMessages receives a non-empty projected history, the last loop iteration has no messages[i + 1]; this new expectDefined throws before the existing next ? ... : null fallback can run. That makes ordinary one-message histories, and the final message of longer histories, fail during Control UI/session-history projection instead of simply skipping the heartbeat-pair check as before.
Useful? React with 👍 / 👎.
| targetAgent.models = { | ||
| ...targetAgent.models, | ||
| [params.modelRef]: structuredClone(preparedAgent.models[params.modelRef]), | ||
| ...expectDefined(targetAgent.models, "agent models"), |
There was a problem hiding this comment.
When app-guided setup prepares per-agent model metadata, such as a Codex route pinning agentRuntime, and the user's existing agents.list entry has no models object, this spread now throws. The previous ...targetAgent.models treated the missing container as {}, so sparse existing agent configs abort after a successful probe instead of persisting the selected model/runtime metadata.
Useful? React with 👍 / 👎.
clawsweeper
Bot
added
rating: 🧂 unranked krab
eb30aca to
31a233d
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 31a233d9bd
ℹ️ 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".
| modelProvider: merged.storeEntry?.modelProvider, | ||
| model: merged.storeEntry?.model, | ||
| usage, | ||
| usage: expectDefined(usage, "session usage summary"), |
There was a problem hiding this comment.
When a listed session has no in-range cost summary, usageByEntryIndex remains null; the endpoint previously returned usage: null, and src/shared/usage-types.ts explicitly allows SessionUsageEntry.usage to be SessionCostSummary | null with UI coverage for null rows. This expectDefined now throws for ordinary new/empty sessions, causing sessions.usage to fail instead of returning the row to the Usage/Profile pages.
Useful? React with 👍 / 👎.
Part 1/3 of the src NUIA phase-3b burn-down (#104600): iteration and destructuring over index reads, boundary guards on parsed input, and named invariants. Config path walkers bind the path head once; SQLite migration key handling is hoisted without query-shape changes.
…curity, shared Part 2/3: argv/token selection restructured, gateway event/attachment invariants named, security parsers stay fail-closed (invariant violations throw), edit-distance matrices access checked entries.
Part 3/3: channels, plugins, process, cron, plugin-sdk, media, logging, tui, hooks, daemon, and small directories. Latent bug fixed: a tailnet resolver could leak undefined through a string|null contract and now fails with a descriptive local error.
Review findings: expectDefined misused where absence is a legitimate state. CLI --profile/route-args missing next tokens take their existing miss paths; help normalization compares --help against the last positional again; first-time plugin install spreads absent cfg.plugins; denylist scan iterates manifest dependency entries instead of throwing on omitted sections; tailnet resolver returns a guaranteed string at the source instead of a caller-side undefined throw.
…roughs PROVIDER_LABELS becomes a satisfies-typed closed record (static reads provably defined; dynamic lookups go through providerUsageLabel with honest string|undefined). Status-scan overview passes its optional params through unchanged instead of asserting them.
The original signature claimed ChatChannelMeta while leaking undefined on bundled channel id metadata drift; three of four callers already handled absence. The return type now says so, and the one assuming caller falls back to the raw channel label.
Covers the sqlite-sessions flip and auth-source-plan code that landed mid-phase, plus the channel-validation test consuming the now honestly optional getChatChannelMeta.
getChatChannelMeta keeps its shipped plugin-SDK signature (defined for bundled ids, fail-loud on impossible misses); new findChatChannelMeta carries the drift-tolerant optional contract for core auto-enable and formatting paths.
…ed catalog lookup qa-channel spread getChatChannelMeta over an id that is never in the bundled catalog, shipping an empty setup meta by accident; the fail-loud SDK accessor exposed it. The channel now declares its metadata once.
…cript tail expectDefined wrapped messages[i + 1] whose absence on the final message is the normal case; the adjacent ternary already handled it. Restores the plain optional read with an explicit guard in the pair condition.
… ids again createChannelPluginBase spreads bundled catalog meta for ANY channel id, where absence is the normal case for external plugins; the resolver is honestly optional again while the exported bundled-id accessor keeps the fail-loud contract.
Fresh-setup and first-install paths (crestodian setup inference, hook
installs, agent config base, target agent models) legitimately lack the
section being rebuilt; spreading undefined is the shipped {} semantics.
Removes the remaining gratuitous assertion wraps found by tree audit.
31a233d to
eb37660
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: eb376605b3
ℹ️ 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 { provider, displayName: PROVIDER_LABELS[provider], windows }; | ||
| return { | ||
| provider, | ||
| displayName: expectDefined(providerUsageLabel(provider), "gemini provider usage label"), |
There was a problem hiding this comment.
Because fetchGeminiUsage is exported from src/plugin-sdk/provider-usage.ts and UsageProviderId/manifest usageProviders are arbitrary strings, an external provider that reuses this helper with its own id outside PROVIDER_LABELS will now throw here after a successful quota response. That converts a valid usage snapshot into a fetch failure; keep the existing display-name fallback behavior, e.g. providerUsageLabel(provider) ?? provider or resolveProviderUsageDisplayName(provider).
Useful? React with 👍 / 👎.
|
Merged via squash.
|
…ase 3b) (openclaw#104773) * fix(core): make indexed access explicit in auto-reply, infra, and config Part 1/3 of the src NUIA phase-3b burn-down (openclaw#104600): iteration and destructuring over index reads, boundary guards on parsed input, and named invariants. Config path walkers bind the path head once; SQLite migration key handling is hoisted without query-shape changes. * fix(core): make indexed access explicit in cli, gateway, commands, security, shared Part 2/3: argv/token selection restructured, gateway event/attachment invariants named, security parsers stay fail-closed (invariant violations throw), edit-distance matrices access checked entries. * fix(core): make indexed access explicit across remaining src surfaces Part 3/3: channels, plugins, process, cron, plugin-sdk, media, logging, tui, hooks, daemon, and small directories. Latent bug fixed: a tailnet resolver could leak undefined through a string|null contract and now fails with a descriptive local error. * fix(core): keep optional boundaries optional after per-commit review Review findings: expectDefined misused where absence is a legitimate state. CLI --profile/route-args missing next tokens take their existing miss paths; help normalization compares --help against the last positional again; first-time plugin install spreads absent cfg.plugins; denylist scan iterates manifest dependency entries instead of throwing on omitted sections; tailnet resolver returns a guaranteed string at the source instead of a caller-side undefined throw. * refactor(core): closed-key provider labels and honest optional passthroughs PROVIDER_LABELS becomes a satisfies-typed closed record (static reads provably defined; dynamic lookups go through providerUsageLabel with honest string|undefined). Status-scan overview passes its optional params through unchanged instead of asserting them. * fix(channels): make getChatChannelMeta honestly optional The original signature claimed ChatChannelMeta while leaking undefined on bundled channel id metadata drift; three of four callers already handled absence. The return type now says so, and the one assuming caller falls back to the raw channel label. * fix(core): index-safety for post-rebase main drift Covers the sqlite-sessions flip and auth-source-plan code that landed mid-phase, plus the channel-validation test consuming the now honestly optional getChatChannelMeta. * refactor(channels): split chat-meta accessors along the SDK contract getChatChannelMeta keeps its shipped plugin-SDK signature (defined for bundled ids, fail-loud on impossible misses); new findChatChannelMeta carries the drift-tolerant optional contract for core auto-enable and formatting paths. * fix(qa-channel): own channel metadata instead of a guaranteed-undefined catalog lookup qa-channel spread getChatChannelMeta over an id that is never in the bundled catalog, shipping an empty setup meta by accident; the fail-loud SDK accessor exposed it. The channel now declares its metadata once. * fix(gateway): heartbeat projection lookahead is optional at the transcript tail expectDefined wrapped messages[i + 1] whose absence on the final message is the normal case; the adjacent ternary already handled it. Restores the plain optional read with an explicit guard in the pair condition. * fix(plugin-sdk): channel plugin factory tolerates non-bundled channel ids again createChannelPluginBase spreads bundled catalog meta for ANY channel id, where absence is the normal case for external plugins; the resolver is honestly optional again while the exported bundled-id accessor keeps the fail-loud contract. * fix(core): spreads of optional config sections stay optional Fresh-setup and first-install paths (crestodian setup inference, hook installs, agent config base, target agent models) legitimately lack the section being rebuilt; spreading undefined is the shipped {} semantics. Removes the remaining gratuitous assertion wraps found by tree audit.
1 participant
Related: #104600
What Problem This Solves
Phase 3b of the
noUncheckedIndexedAccessadoption (#104600): the remaining 669 unchecked-index errors across all ofsrc/**outsidesrc/agents— auto-reply, infra, config, cli, gateway, commands, security, and 20 smaller directories. With this landed, the entire core program (src + packages) is index-safe and ready for the flag flip in phase 3c.Why This Change Was Made
Pure burn-down to zero (repo probe 669 → 0), 234 files, eight commits (including mid-flight rebases across the sqlite-sessions flip and the pre-2026.4 shim removal, whose new code was made index-safe here too). The fix ladder from prior phases applied: iteration over indexing, boundary guards on genuinely-optional input, named invariants (
expectDefined), closed-key tightening.This chunk needed real review pressure, documented here honestly:
expectDefined(..., "indexed value")escape hatches; a rework round eliminated every generic context (72 restructured away, 65 converted to real guards, the rest given site-specific contexts) and removed introduced casts/assertions.--profile/route-args missing next tokens must take their existing miss paths (the "rejects missing profile value" test proves absence is input, not invariant); nestedhelp X Y --helpnormalization must compare against the last positional; first-time plugin install must tolerate an absentcfg.plugins; dependency-denylist scanning must skip omitted manifest dependency sections (the guard was one line below the throw); status-scan must pass its optional params through unchanged.getChatChannelMetahonestly optional, which would have changed a shipped plugin-SDK signature (two bundled extensions consume it). Final shape preserves the SDK contract (defined for bundled ids, fail-loud on impossible misses) and addsfindChatChannelMetafor the drift-tolerant core paths — reviewed clean (0.98).stringat the source (guarded first-IP) instead of a caller-side undefined-throw;PROVIDER_LABELSbecame asatisfies-typed closed record (static reads provably defined, dynamic lookups through an honeststring | undefinedhelper), deleting ~10 assertions.User Impact
No behavior change on healthy paths. Plugin install, CLI parsing, and status scanning behave identically including their absence paths (now proven by types instead of convention).
Evidence
pnpm check+pnpm check:test-typesgreen on the final tree; across seven battery runs every completed test file passed (thousands; the only real failures were the seven bugs fixed above), with all aborts being box-level worker/lease deaths under memory pressure — exact-head sharded CI on this PR is the complete-suite arbiter.