Conversation
|
@codex: review |
![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: c3be0511c0
ℹ️ 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".
| // Insert the context prompt just before the trailing user prompt. It is regenerated on every | ||
| // call and not stored in the session, so keeping it at the end preserves the cached prefix of | ||
| // the earlier messages. | ||
| messages = slices.Insert(messages, len(messages)-1, NewTextCompletionMessage(RoleUser, contextPrompt)) |
There was a problem hiding this comment.
When a project defines ai_instructions, this inserts the new contextPrompt as a user message immediately before the actual user prompt, and that prompt now contains the administrator project-wide instructions. That downgrades those instructions from the previous system prompt to user-level content that the following user message can contradict or override, so configured analyst behavior/guardrails can be ignored. Keep admin instructions in the system prompt or another higher-priority message while moving only truly dynamic dashboard context here.
Useful? React with 👍 / 👎.
|
@codex: review again |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bfad59c65f
ℹ️ 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".
| // Insert the context prompt just before the trailing user prompt. It is regenerated on every | ||
| // call and not stored in the session, so keeping it at the end preserves the cached prefix of | ||
| // the earlier messages. | ||
| messages = slices.Insert(messages, len(messages)-1, NewTextCompletionMessage(RoleUser, contextPrompt)) |
There was a problem hiding this comment.
When an analyst completion runs long enough for token truncation after tool calls have been appended, this inserted context message is no longer near the tail; maybeTruncateMessages only preserves the first four messages and a suffix, so it can drop the current dashboard filters/time range/date while keeping later tool-call history. In large dashboard sessions, later iterations can therefore query or answer without the per-turn context that previously lived in the always-kept system prompt.
Useful? React with 👍 / 👎.
nishantmonu51
left a comment
nishantmonu51
left a comment
There was a problem hiding this comment.
The branch is 209 commits behind main and git merge-tree reports content conflicts in both runtime/ai/ai.go and runtime/ai/analyst_agent.go, so a rebase is needed before this is mergeable.
Two smaller things:
- The truncation indicator is emitted with role
system, and the Claude driver'sconvertMessageslifts everysystemmessage into the system block, so on Claude the "[N messages omitted]" text mutates the cached system prefix each time the skip count steps. This is pre-existing onmain, but it works directly against this PR's caching goal. - The
admindriver's newmax_input_tokensspec property is unreachable: its config is built from a fixed map inadmin/deployments.go:270andcli/pkg/local/app.go:281(admin_url,access_token,project_id), so only the 200k default can ever apply.
| for keptTokens > maxTokens && truncateKeepFirst+skipped < len(messages)-1 { | ||
| keptTokens -= est[truncateKeepFirst+skipped] | ||
| skipped++ | ||
| } | ||
|
|
||
| if len(messages) <= maxMessages { | ||
| if skipped == 0 { | ||
| return messages | ||
| } | ||
|
|
There was a problem hiding this comment.
Once skipped goes above zero, the round-up to truncateStep and the len(messages)-truncateKeepFirst-1 cap together keep only the final message for any conversation shorter than about 25 messages, so a single turn can lose the user's question, the context prompt and every tool result. On the first turn of an explore the seeded messages are [system, call, result, call, result, context, user] and the loop then appends (call, result) pairs of up to maxMessageSizeBytes each; query results at the default 250-row limit run 30-60KB, so proto.Size/3 crosses the 136k default budget after roughly nine iterations, at len = 25. The minimal skip of 1 rounds up to 20, the cap keeps only index 24, and the unbalanced-pair cleanup then drops the seed call at index 3 and that last result, leaving the model [system, seed call, seed result, "[20 messages omitted]"]. main cannot reach this state because count-based truncation at maxMessages = 100 with keepLast = 16 never fires within one turn; the step rounding needs a floor that preserves a recent window, and the context and user prompts should be pinned rather than relying on their position.
| // Build completion messages. | ||
| // The system prompt contains only instructions that stay stable for the duration of a session; | ||
| // per-turn dynamic info (date, dashboard state, etc.) goes in a separate context message near the | ||
| // end, so the LLM prompt cache prefix stays valid across turns. | ||
| systemPrompt, err := t.systemPrompt(ctx, args) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| contextPrompt, err := t.contextPrompt(ctx, metricsViewNames, args) |
There was a problem hiding this comment.
main has since landed #9645, which already splits the analyst prompt into a stable systemPrompt() and a per-turn userPrompt(ctx, metricsViewNames, args), with #9851 layered on top, so contextPrompt and the template rewrite here duplicate work that is already merged and should be dropped on rebase rather than reconciled. The slices.Insert(messages, len(messages)-1, ...) on line 207 also does not survive a mechanical rebase: on current main the layout ends [..., user prompt, seeded tool calls], so the last message is a tool result and the insert would land between a seeded tool call and its result.
|
@nishantmonu51 Will close this PR since #9645 handled most of the improvements. |
2 participants
max_input_tokensconfigurable per connector.prompt_cache_keyfor per session cache for openai connector.Checklist: