Upstream auto approval review - #45
Conversation
💡 Codex ReviewWhen the prompt leaves fewer tokens than opendroid/app/src/main/java/com/opendroid/ai/core/llm/PromptBudget.kt Lines 22 to 23 in 0c1ad3a For prompts containing CJK text, emoji, or token-dense JSON, dividing UTF-16 character count by four substantially underestimates the tokenizer output—for example, 1,000 CJK characters are reported as only 250 tokens. Such a prompt can pass opendroid/app/src/main/java/com/opendroid/ai/core/service/OpenDroidService.kt Lines 138 to 140 in 0c1ad3a When ℹ️ 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". |
Confidence Score: 5/5Safe to merge — the checkbox key fix is a targeted correctness improvement with no behaviour change elsewhere, and the new test documents existing behaviour without modifying any production logic. Both changes are small and well-scoped. The Files Needing Attention: No files require special attention. The only open question is whether a plan composed entirely of blank-action steps should auto-approve or be held for manual review, but this is a pre-existing design decision that the PR merely documents with a test.
|
| Filename | Overview |
|---|---|
| app/src/main/java/com/opendroid/ai/ui/screens/ChatScreen.kt | Adds planId parameter to ProposedPlanPrompt and uses it as a remember key alongside blockedActions to prevent checkbox state from leaking between plans that share the same blocked action list. |
| app/src/test/java/com/opendroid/ai/core/agent/AutoApprovalPolicyTest.kt | Adds a test asserting a step with a blank primary action is auto-approved with an empty granted set; the assertion is technically correct but documents a subtle edge case where a fully-blank-action plan is treated as having no blocked actions and thus auto-approves. |
Sequence Diagram
sequenceDiagram
participant VM as ViewModel
participant CS as ChatScreen
participant APP as AutoApprovalPolicy
participant PPP as ProposedPlanPrompt
VM->>CS: AgentState.PlanProposed(plan)
CS->>APP: blockedActions(granted, plan.steps)
APP-->>CS: "blocked: List<String>"
CS->>PPP: "planId, goal, blockedActions=blocked, ..."
Note over PPP: remember(planId, blockedActions)<br/>resets checkedGrants when<br/>either key changes
PPP-->>CS: onApprove(grants) / onReject()
CS->>VM: approvePlan(grants) / rejectPlan()
Reviews (2): Last reviewed commit: "Merge branch 'main' into fix/upstream-au..." | Re-trigger Greptile
One conflict, in ProposedPlanPrompt's checkedGrants state. Both sides
replaced the same unkeyed `remember { }`: main keyed it on
`blockedActions`, this branch keyed it on the newly threaded `planId`.
Resolved by keying on both. `planId` is the identity key — two different
plans that happen to block the same actions must not share ticked
grants, which keying on `blockedActions` alone would allow. But the
checkboxes are rendered from `blockedActions`, so a change to that list
also has to clear ticks that now refer to rows which are gone. Neither
key subsumes the other.
|
Merged One conflict, in Resolved by keying on both, because neither key subsumes the other. var checkedGrants by remember(planId, blockedActions) { mutableStateOf(setOf<String>()) }Verified locally before pushing: |
No description provided.