Repository navigation
fix(scan): reject oversized plan prompts before sending them - #1741
RerankerGuo wants to merge 1 commit into
Conversation
alibaba#1652 (merged 2026-10-12) added a local pre-flight token gate to the MAIN_TASK phase in executeSubtask, and the cmd-level test TestExecuteScanPromptOverflowCannotReportClean relies on that gate to keep the LLM request count at zero for an overflowing scan. The default scan template (`internal/config/template/scan_template.json`) ships a non-empty PLAN_TASK, and scan's plan phase renders the same file content into its prompt as the main phase. The plan call had no analogous gate, so an overflowing file with plan enabled burned one doomed provider request before the main-task guard rejected the file locally. Default config therefore does NOT match the "zero LLM requests" guarantee the new cmd test asserts (it passes only because the test uses `--no-plan`). This applies the same gate to the plan phase: count plan messages locally, compare against the shared PromptTokenLimit (80% of MaxTokens, the same threshold MAIN_TASK uses), and on overflow emit the existing token_threshold_exceeded telemetry + warning and fall through to the noPlan sentinel instead of dispatching. Plan still falls back to plan-less, so the file's chance of being reviewed is unaffected when the main prompt fits. Closes the gap between alibaba#1652 and the default scan config.
|
✅ OpenCodeReview: Review complete: 0 finding(s) across 1 selected item(s). |
kittimzhe
left a comment
kittimzhe
left a comment
There was a problem hiding this comment.
Verified end to end — the gate mirrors the MAIN_TASK site faithfully (same CountMessagesTokens → PromptTokenLimit comparison, same token_threshold_exceeded warning + telemetry, (skipping plan) disambiguating the log), it returns noPlan before the session record append so no phantom task record, and the plan-less fallback keeps the file reviewable when only the plan prompt overflows. Both test cases pin the right invariants — plan-overflows-main-fits still gets its one review call, both-overflow stays at zero provider requests with the all-files-failed error. Ran the suite locally: green.
The gap analysis in the commit message checks out too: the default scan template ships a non-empty PLAN_TASK rendering the same file content, so without this gate the default config does burn one doomed request per overflowing file, and the cmd-level zero-request guarantee only held under --no-plan.
One non-blocking thought: now that default config matches the guarantee, the cmd-level TestExecuteScanPromptOverflowCannotReportClean could drop its --no-plan and pin the end-to-end zero-request behavior under the default template — that would catch a future regression where the two gates diverge (e.g. someone changes the plan render without the gate). Happy to leave it as a follow-up.
|
I’d prefer reporting the PLAN prompt overflow as an explicit error for the affected file, with a message suggesting Since That could spend tokens on a review the user would not have chosen to run without planning. I’d rather give users control over that tradeoff: reject the oversized PLAN request locally, mark the affected file as failed, and let them decide whether to increase |
This branch has not been deployed
3 participants
Summary
Closes a gap in the recently merged #1652. That PR added a local pre-flight token gate to scan's
MAIN_TASKphase and a cmd-level test that asserts an overflowing scan makes zero LLM requests — but the test passes only because it sets--no-plan. The default scan template (internal/config/template/scan_template.json) ships a non-emptyPLAN_TASK, andmaybeRunPlanrenders the same file content into the plan prompt as the main phase, with no analogous gate. So an overflowing file with the default config still burned one doomed PLAN call beforeMAIN_TASKrejected the file locally.The
PromptTokenLimithelper ininternal/llmloop/compression.goeven documents itself as "shared by the agent and scan pre-flight gates" — the plan phase was the missing consumer.Changes
internal/scan/agent.go—maybeRunPlan: after rendering the plan messages but before theLLMClientcall, count the plan messages, compare against the sharedPromptTokenLimit(80% ofMaxTokens, the same thresholdMAIN_TASKuses), and on overflow emit the existingtoken_threshold_exceededtelemetry + warning and return thenoPlansentinel so the file is still reviewed when the main prompt fits. The doomed provider request never goes out.Testing
TestRun_PromptOverflowSkipsDoomedPlanRequestcovers "plan overflows, main fits" (plan skipped, file still reviewed) and "plan and main overflow" (zero LLM requests under default config, matching the fix(scan): record rendered prompt overflow as a failed review #1652 cmd-level test's assertion without--no-plan).TestRun_PromptOverflowRecordsFailurestill green: the new gate shares the warning type andPromptTokenLimitwith the main-task gate, so the assertion in that test (warning + session failure record) keeps working.go test -count=1 ./internal/scan/— all green locally.go vet,gofmt -s,verify-license.sh,verify-english-only.goclean.Closes #1740