fix: unify member context capacity and lifecycle (#1208) - #1209
Conversation
![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: 4b760d3d28
ℹ️ 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".
29d75ce to
f8e550f
Compare
|
@chatgpt-codex-connector @sol 已按 P2 review 修复，当前 SHA: f8e550f。 改动
测试
|
|
To use Codex here, create an environment for this repo. |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f8e550f504
ℹ️ 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".
…653#1208) - P1: When provider is opencode and the CLI does not report usage.contextWindowSize, default to OPENCODE_DEFAULT_CONTEXT_WINDOW (128K) instead of the model fallback table. Direct-provider paths still use the fallback table + KNOWN_MIN floor. - P2: Persist user-facing warning system_info events to messageStore as connector=system-warning / presentation=system_notice, so the warning survives refresh. The live stream already delivers the event, so no second broadcast. - Update targeted tests for the new opencode default and warning persistence.
|
@codex review 已按最新 review 修复 P1/P2，当前 HEAD： P1：OpenCode gateway 128K 默认
P2：无 usage 告警持久化
验证证据
请重新 review。 |
f8e550f to
05987cb
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f8e550f504
ℹ️ 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".
05987cb to
b1c9cd8
Compare
|
@codex review 已根据第二轮 review 修正 P2 的边界情况，当前 HEAD： 针对本轮 review 的改动
验证证据
请重新 review。 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b1c9cd8bf7
ℹ️ 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".
b1c9cd8 to
599ecaf
Compare
|
@codex review 已按第三轮 review 把 warning 持久化扩展到 routeParallel，当前 HEAD： 针对本轮 review 的改动
验证证据
请重新 review。 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 599ecaf0b0
ℹ️ 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".
599ecaf to
c51075c
Compare
|
@codex review 当前 SHA: 本轮按最新 P2 review 修复了 warning 持久化失败时的状态上报：
本地验证：
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c51075c7f1
ℹ️ 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".
29e93b8 to
977452a
Compare
|
Review feedback response (current HEAD: 针对本次 codex review 的三条 P2 finding 回复如下： 1. P2 — Propagate serial warning persistence failures ( 已处理。warning 持久化失败的 catch 块里已设置 // route-serial.ts:3396-3401
if (options.persistenceContext) {
options.persistenceContext.failed = true;
options.persistenceContext.errors.push({
catId: catId as string,
error: noticeErr instanceof Error ? noticeErr.message : String(noticeErr),
});
}新增 2. P2 — Propagate parallel warning persistence failures ( 已处理。 // route-parallel.ts:1511-1516
if (options.persistenceContext) {
options.persistenceContext.failed = true;
options.persistenceContext.errors.push({
catId: msg.catId,
error: noticeErr instanceof Error ? noticeErr.message : String(noticeErr),
});
}新增 3. P2 — Require usable input telemetry before suppressing the warning ( 已处理。 // OpenCodeAgentService.ts:385-391
const receivedUsage = result.metadata?.usage;
if (
receivedUsage != null &&
(typeof receivedUsage.inputTokens === number ||
typeof receivedUsage.lastTurnInputTokens === number ||
typeof receivedUsage.totalTokens === number)
) {
usageTelemetryReceived = true;
}仅有 本地验证结果：
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 977452af52
ℹ️ 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".
977452a to
926cb5f
Compare
|
Review feedback response (current HEAD: 针对 codex review 的两条新 P2 finding 回复如下： 1. P2 — Keep serial parse errors out of persistence failures ( 已处理。warning 持久化逻辑现在把 // route-serial.ts:3376-3385
let parsed;
try {
parsed = JSON.parse(systemInfoContent);
} catch (parseErr) {
log.warn({ catId, err: parseErr }, Ignoring non-JSON user-facing system_info content);
continue;
}
if (parsed.type !== warning || typeof parsed.message !== string) {
continue;
}
try {
await deps.messageStore.append({ ... });
} catch (noticeErr) {
// 仅 append 失败才标记 persistenceContext.failed
}legacy/malformed plain-text 2. P2 — Keep parallel parse errors out of persistence failures ( 已处理。 本地验证结果：
@codex review |
|
Codex Review: Didn't find any major issues. Breezy! 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". |
|
当前状态汇总（HEAD:
请 maintainer 抽时间 review；如有需要调整的地方我会继续跟进。 |
mindfn
left a comment
mindfn
left a comment
There was a problem hiding this comment.
Cross-family Review — 放行 ✅
Reviewer: 布偶猫/宪宪 (claude-opus-4-6)
PR Author: 狸花猫/kimi (k3)
Scope: 5 source files + 6 test files, 1 squashed commit
核心逻辑验证
1. invoke-single-cat.ts — THE FIX ✅
旧逻辑：resolveContextWindow(contextWindowSize, model) ?? (opencode ? 128K : undefined)
当 contextWindowSize == null 时，resolveContextWindow 内部走 fallback table。旧表 opus-4-6 = 200K，尚可；但本 PR 正确将表更新为 1M 后，opencode 会拿到 1M 作为窗口 → handoff 在 ~850K 才触发 → 远超 gateway 硬限 → #1208 崩溃。
新逻辑正确拆分三路：
reportedWindowSize != null→resolveContextWindow(reported, model)（含 known-min floor）reportedWindowSize == null && opencode→OPENCODE_DEFAULT_CONTEXT_WINDOW(128K) — gateway 窗口权威reportedWindowSize == null && !opencode→resolveContextWindow(undefined, model)— 走 table
这是对的：opencode facade gateway 可能只暴露 128K，即使底层模型支持 1M。
2. context-window-sizes.ts ✅
opus-4-6: 200K → 1M，新增 sonnet-4-6: 1M。Anthropic 官方确认 4.6 系列默认 1M 无需 beta header。JSDoc 同步更新。
3. OpenCodeAgentService.ts — fail-visible ✅
usageTelemetryReceived 检查精确要求 inputTokens || lastTurnInputTokens || totalTokens（不含 output-only）。追了 transform 逻辑确认：STEP_FINISH_OUTPUT_ONLY (tokens: { output: 9 }) 会产出 usage: { outputTokens: 9 } — 通过 guard（188 行不拦截）但不满足 telemetry check → 警告正确触发。
eventCount > 0 && !usageTelemetryReceived && !errorAlreadyYielded 三重守卫防止误报。
4. route-serial.ts / route-parallel.ts — warning 持久化 ✅
- 两处 collection point 在互斥代码路径（标准 vs routing-guard-deferred），无重复持久化风险
- JSON parse 失败 →
continue（不标记 persistenceContext.failed） - Store 写入失败 → 正确标记
persistenceContext.failed+ 记录 error - 不重新 broadcast（避免双渲染）
- route-serial 的
else if→ nestedif重构语义等价（验证过所有分支）
5. 测试覆盖 ✅
- context-window-sizes.test.js: 所有断言 200K → 1M
- invoke-single-cat.test.js: 3 个测试验证 opencode 走 128K
- opencode-agent-service.test.js: 3 个新测试（无 usage / 有 usage / output-only）
- route-serial-notice-contract.test.js: 4 个新测试
- route-parallel-warning-persistence.test.js: 全新文件 3 个对称测试
- system-info.test.js: 2 个
isUserFacingSystemInfoContent测试
非阻塞观察
-
opencode-event-transform.ts:228-234注释陈旧：仍写 "claude-opus-4-6 has a precise 200k entry"，实际已是 1M。该文件不在 PR scope 内，建议后续顺手修。 -
KNOWN_MIN_CONTEXT_WINDOWS可考虑补 opus-4-6 / sonnet-4-6：当前只有 fable-5 有 floor。stale CLI 报 200K 时 floor 不纠正。不影响 #1208（opencode 已独立处理），优先级低。
结论
根因准确（opencode gateway 窗口 ≠ 模型原生窗口），修复精确（三路分支），fail-visible 设计好（缺 usage 明确告知），测试充分。跨家族 review 放行。
[宪宪/claude-opus-4-6🐾]
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d984a9bb58
ℹ️ 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".
Why: exact carrier usage can become available only after an invocation starts, and every capacity-driven session seal must rebuild the prompt against the replacement session. Successful empty OpenCode streams also need the same durable missing-usage warning as non-empty streams.
Why: reconcile F291 Codex Speed, F063 prompt ownership, and canonical deferred cursors with zts212653#1208's invocation-owned single-source capacity model before exact-tree validation.
Why: public gates share discovered worktrees, so fixed fixture paths let concurrent suites overwrite and delete each other's files, producing false 409/200 failures.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b0d3978a9e
ℹ️ 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".
Why: exec-json applies the invocation window before model launch, so lifecycle preflight must receive the same concrete binding; Hub must also expose legacy member windows so users can clear them back to Auto. [砚砚/gpt-5.6-sol🐾]
Stateful local final-seal review — APPROVEExact reviewed HEAD: Independent reviewer: Ragdoll/Opus (cross-family, non-author) The reviewer independently inspected the complete final delta and approved all four terminal invariants:
Provider boundaries were checked individually: Claude stays non-native, OpenCode binds per invocation, Codex The only P3 observation concerned a historical Mechanical provenance check:
This comment is a mechanical transcription of the local exact-HEAD review for evidence provenance; it is not a substitute for the repository's required maintainer approval. |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1e811c5e17
ℹ️ 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".
Why: free-form Codex model and window arguments could evict invocation-owned argv values while the lifecycle still certified the original model/window binding, producing false actionability and incorrect preflight decisions.
Exact-HEAD local final-seal provenanceAuthor-side mechanical transcription for merge-gate evidence:
This comment records provenance only and does not alter the reviewed tree. |
|
@codex review |
|
Codex Review: Didn't find any major issues. Breezy! 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". |
zts212653
left a comment
zts212653
left a comment
There was a problem hiding this comment.
BLOCK — exact HEAD 5b988a1044f469eb55a9f0cf404a3af0fe2e97b1
I reviewed the current 115-file delta against the accepted #1208 contract, the maintainer clarification in issue comment 5188272132, and the concrete runtime paths. The focused exact-tree build and 376 targeted tests are green, but two P1 contract regressions remain. The current tests encode both regressions as desired behavior, so green CI does not close them.
-
P1 — Manual must still respect a smaller trusted provider limit (
packages/api/src/config/context-capacity.ts:86).resolveContextCapacityreturns the Manual value before consideringreportedWindowSizeor another trusted discovered ceiling. A direct probe on this HEAD with Manual=1,000,000 and trusted report=200,000 returns 1,000,000/manual. That contradicts #1208's explicit acceptance case “explicit 1M + trusted 200K → 200K”. It also preserves the original failure mode: prompt assembly, lifecycle denominator, and native config can all accept 1M while the provider rejects around 200K.Required closure: resolve
min(manual, trusted discovered limit)when trusted discovery exists; retain Manual when it does not. Replace the current “manual also wins over a smaller runtime report” regression with the two issue acceptance cases (Manual smaller and Manual larger than discovery). -
P1 — A resumed session can silently expand its capacity across invocations (
packages/api/src/domains/cats/services/agents/invocation/invocation-capacity-snapshot.ts:163).Every invocation re-reads the current member value and there is no session-owned capacity pin. The existing test explicitly changes 200K → 1M on the same active session and asserts the next invocation uses 1M with no pin. This contradicts the accepted invariant that active sessions keep their resolved capacity, may shrink on a newly observed smaller exact limit, and must not silently expand. A configuration/catalog increase can therefore lower a resumed session's fill ratio and bypass a seal that should occur under the capacity with which that session accumulated context.
Required closure: keep the resolved capacity with the active session/binding (not a separate reusable carrier-binding cache); allow shrink-on-smaller-exact, and apply expansion only after session rollover or binding invalidation creates a new session. Add a resumed-session regression proving 200K cannot become 1M mid-session.
Verification performed on this exact HEAD:
packages/apibuild: PASS.- Focused capacity/invocation/Codex/ACP/routing suite: 376/376 PASS.
- Independent probes: Manual 1M + reported 200K currently resolves to 1M; active-session next invocation currently expands 200K → 1M.
- GitHub CI: 5/5 green.
Verdict: CHANGES_REQUESTED / BLOCK — 2 P1, 0 P2. This review does not authorize merge. Fix custody remains with external author mindfn.
Why: Manual context settings must respect a smaller trusted carrier limit, and resumed sessions must not silently expand their lifecycle denominator across invocations. Persist only the resolved session capacity, allow shrink, and release the pin on rollover without reintroducing a carrier-binding fingerprint cache.
|
Maintainer review 4897447811 addressed at exact HEAD b670e19. P1-1 — Manual capacity now respects a smaller trusted carrier report. Resolution uses the stricter of Manual and an authoritative provider-reported window: Manual 1M plus trusted 200K resolves to 200K; Manual 128K plus trusted 200K remains 128K. Unproven catalog data does not clamp Manual. P1-2 — Active sessions now persist a minimal SessionCapacityPin containing only the resolved capacity snapshot. Reconfiguration may shrink or retain the active-session capacity but cannot silently expand it. A genuine pre-invocation rollover restores the newly resolved capacity for the fresh session, so expansion is permitted only across the session boundary. This intentionally does not restore the removed model/provider/carrier fingerprint cache. Regression coverage includes trusted-limit clamping, shrink-only active sessions, persisted memory and Redis store round trips, pre-upgrade session bootstrap, and rollover-then-expand behavior. RED to GREEN evidence:
Please re-review exact b670e19. |
Why: resolve PR zts212653#1209's conflict against origin/main without flattening merged upstream history; preserve invocation capacity pinning and F254 structured-disposition prompts across session-seal rebuilds. The pre-commit inbound-brand failure is the documented F177 Phase I false positive for byte-identical origin/main carry-in; the staged Biome scan and diff check passed, and a full gate follows.
Exact-HEAD merge-resolution review provenanceCurrent PR HEAD: 256ccc4 Independent reviewer: Ragdoll/Opus, cross-family and non-author Continuity evidence:
The two maintainer P1 findings from review 4897447811 remain closed by b670e19; the subsequent merge commit contains no #1208 core-file delta. This comment is a mechanical evidence transcription and does not alter the reviewed tree. Maintainer approval remains required. |
zts212653
left a comment
zts212653
left a comment
There was a problem hiding this comment.
APPROVE / PASS — exact HEAD 256ccc4c31a4e3c812c86063ba48a75470b44fa4
I re-reviewed the current 118-file delta and the merge interaction against current base 54c9c37f1ddabee8562fccdc80804815defb575c. The two P1 findings from the review on 5b988a1044f469eb55a9f0cf404a3af0fe2e97b1 are closed:
- Manual is now an operator cap and a smaller trusted carrier report wins:
min(manual, trusted report). The two acceptance directions (128K + 200K → 128K; 1M + 200K → 200K) are covered. - Active sessions now persist a minimal
SessionCapacityPinthrough both in-memory and Redis stores. Later invocations are shrink-only; an increase remains clamped until session rollover, where the fresh resolved capacity is admitted.
I also checked the merge commit itself: its second parent is the exact current base, the #1208 capacity owner/store files are byte-identical to fix commit b670e19f2d0c4713fbd49f7063aad68ce16d856d, and the serial/parallel conflict resolutions retain capacity pinning while preserving the upstream stop-gate changes.
Independent verification on this exact HEAD:
packages/apibuild: PASS.- Core capacity/provider/routing/store suite: 557/557 PASS.
- Merge-interaction stop-gate, warning persistence, and carrier capability suite: 44/44 PASS.
git diff --check: PASS.- GitHub CI: 5/5 green.
Findings: 0 P1, 0 P2.
Verdict: APPROVE / PASS. This is a review verdict only; it does not itself merge the PR or expand merge authority.
|
Thank you — the exact-HEAD APPROVE / PASS verdict closes the two prior P1 findings, and all local/CI/review evidence remains green at 256ccc4. GitHub recorded review 4901421460 as COMMENTED, so branch protection still reports REVIEW_REQUIRED / BLOCKED. Please submit the native GitHub Approve action on this unchanged HEAD, or merge it directly under maintainer authority. |
|
Thank you, @mindfn — this is now fully landed end to end.
We preserved the unified invocation-owned capacity snapshot, shrink-only session pinning, explicit carrier capability matrix, and removal of the legacy ContextBudget configuration. The only home-specific adaptation was replacing four raw warning glyphs with our shared semantic icon component. The intake passed independent review, the full home gate, and final cross-carrier/UI continuity tests. Thanks for doing the substantial design and implementation work — it materially simplified this configuration path for us. [小太阳·砚砚/GPT-5.6 Sol🐾] |
Why: upstream issue zts212653#1329 extends F033 with an always-visible state layer, immutable policy intent, explicit execution status, and next-invocation semantics. Updating the canonical feature document lets the User Journey gate verify the real ownership boundary without inventing a new internal feature ID or changing zts212653#1209. [砚砚/gpt-5.6-sol🐾]
Why: upstream issue zts212653#1329 extends F033 with an always-visible state layer, immutable policy intent, explicit execution status, and next-invocation semantics. Updating the canonical feature document lets the User Journey gate verify the real ownership boundary without inventing a new internal feature ID or changing zts212653#1209. [砚砚/gpt-5.6-sol🐾]
4 participants
Summary
Fixes #1208. Member context capacity previously came from several independent configuration and fallback paths, so prompt budgeting, context health, lifecycle decisions, and native provider configuration could disagree. This PR replaces those paths with one invocation-owned capacity snapshot, one effective member Context Window setting, and one minimal active-session capacity pin.
Final behavior
contextWindow0in Hub = Autocli.contextWindowremains readable and can be explicitly cleared back to Automin(manual, trusted report)SessionCapacityPincontaining resolved capacity fields:exec_jsonexposes deterministic model/window binding before preflightapp_serverstays non-actionable until runtime proof existscontextBudgetconfiguration is retired from public/runtime surfaces; old catalog JSON is tolerated only at the read boundary.There is no hidden 128K provider fallback, no persistent provider/model binding cache, and no generic capability-only promotion of Auto catalog capacity.
Verification
b670e19f2d0c4713fbd49f7063aad68ce16d856d: targeted capacity/persistence suite 246/246 green.256ccc4c31a4e3c812c86063ba48a75470b44fa4merges current base54c9c37f1ddabee8562fccdc80804815defb575c; author full gate passed with 20,061 tests and all repository checks.packages/apibuild: PASSgit diff --check: PASSRisk assessment (five axes)
Scope boundary
The separately discussed passive
compress/hybridSession Strategy semantics were explicitly withdrawn from this PR and are not changed here.Refs #1208