Repository navigation
WIP: feat(tower): limit TowerStatus max entries - #4427
tpoisonooo wants to merge 3 commits into
Conversation
🦋 Changeset detectedLatest commit: fa3927d The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
commit: |
![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.
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| await targetProfile.setModel(binding.model); | ||
| if (binding.thinking !== undefined) targetProfile.setThinking(binding.thinking); |
There was a problem hiding this comment.
When a stopped subagent is resumed onto a pool alias and [secondary_model].default_effort is unset, binding.thinking is undefined, so this branch changes only the model and preserves the resumed agent's previous effort. For users relying on per-model variants, such as moving a high-effort agent to an alias whose model entry defaults to max, this silently ignores the selected alias's documented default; fresh spawns correctly call resolveSubagentThinking(...) to apply the model-entry/global fallback chain. Resolve the target model and apply that same effort resolution before running the resumed agent.
AGENTS.md reference: AGENTS.md:L130-L132
Useful? React with 👍 / 👎.
| if ( | ||
| mission.kind === 'artifact' && | ||
| (selectedReview.reviewer !== 'tower' || selectedReview.mission !== mission.id) | ||
| ) { |
There was a problem hiding this comment.
When an artifact branch advances after TowerComplete creates its acceptance request—for example, a resumed worker commits another deliverable before the human responds—the request still lists the old paths, but TowerReview stamps the branch's current tip and this check accepts it solely from the reviewer and mission identity. The human can therefore approve the stale report while the subsequent exact-tip review silently authorizes additional, never-presented files for merge. Persist the completion tip/path manifest and require tower acceptance to reference that same tip, or regenerate and present the current manifest before recording acceptance.
AGENTS.md reference: AGENTS.md:L130-L132
Useful? React with 👍 / 👎.
| const base = await this.diffBase(state, beforeMission); | ||
| const changed = await diffNameOnly(this.repoRoot, base, beforeMission.branch); | ||
| const missing = changed.filter((path) => !report.includes(path)); | ||
| if (missing.length > 0) { |
There was a problem hiding this comment.
For an unchanged artifact branch, substring matching can still let a committed path disappear from the human-facing manifest: if the diff contains both reports/result and reports/result.csv, a report that names only reports/result.csv satisfies report.includes("reports/result"). TowerComplete then succeeds and the omitted file can be accepted and merged without ever being listed. Parse a structured path list or compare delimited, normalized entries exactly rather than searching the free-form report text.
AGENTS.md reference: AGENTS.md:L130-L132
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fa3927d454
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if ( | ||
| mission.kind === 'artifact' && | ||
| (selectedReview.reviewer !== 'tower' || selectedReview.mission !== mission.id) | ||
| ) { |
There was a problem hiding this comment.
When Tower runs in auto/noninteractive mode, any tower-authored clean/merge review is treated as human acceptance even though TowerReview records no user response and the gate checks only reviewer and mission identity. This contradicts tower-mode-full-reminder.md, which requires such runs to leave the artifact waiting, and allows an automatic or mistaken tool call to silently authorize the artifact merge. Reject artifact acceptance while interaction is unavailable, or require a verifiable user-consent record before this gate passes.
AGENTS.md reference: AGENTS.md:L130-L132
Useful? React with 👍 / 👎.
| await targetProfile.setModel(binding.model); | ||
| if (binding.thinking !== undefined) targetProfile.setThinking(binding.thinking); |
There was a problem hiding this comment.
When a background resume specifies model while max_running_tasks is already exhausted, launch() permanently applies this model/thinking change before registerTask() rejects the call. The registration failure path only aborts the attempted run, so the tool returns Too many background tasks are already running while the stopped agent silently remains rebound; retrying later without model then uses a model the failed call was never supposed to commit. Reserve the task slot before rebinding, or restore the previous binding when registration fails.
Useful? React with 👍 / 👎.
| | 'abandoned'; | ||
|
|
||
| export type TowerMissionKind = 'build' | 'survey'; | ||
| export type TowerMissionKind = 'build' | 'survey' | 'artifact'; |
There was a problem hiding this comment.
After adding artifact as a third mission kind, the views used to inspect existing missions still render artifacts exactly like builds: renderMissions() in statusTool.ts only marks surveys, and renderMissionsIndex()/renderMissionFile() in store.ts likewise omit the kind except for surveys. Consequently /tower status and TowerMission(id) cannot tell a human or resumed coordinator that a planned or active mission requires human acceptance rather than independent review; only the one-time TowerPlan result exposes it. Add a kind column or an artifact marker to each persisted/status view.
Useful? React with 👍 / 👎.
tpoisonooo
changed the title
TowerStatus max entriesTowerStatus max entries
Requirement or Bug
TowerStatus结果几千行，改成默认末尾 20 个，允许 LLM 配查询参数。TowerStatus仍然是全量结果，这只是 for LLMBug Reproduction Steps
Root Cause
Code Changes
Behavior Changes and Affected Users
Checklist
/approve).gen-changesetsskill, or this PR needs no changeset.gen-docsskill, or this PR needs no doc update.