Conversation
…r posts The delivery budget added in #2904 was a parallel projection: projectedSeconds re-derived the spaced split, the warmup peel and the warmup wait, all decided elsewhere, and two pre-merge fix commits were drift this shape caused. It also under-charged the plan it modeled: a post that replaces selects with its own synthesize record (RunnerSynthesizedTextEntry.m), which the estimate counted as one call. SynthesizedTextPlan now carries the posts a command makes (characters, whether the post selects first, the wait after it), the budget charges the sum over those steps, and both synthesized routes execute that same array — the loop sleeps the plan's own pauses instead of re-deriving delaySeconds > 0 twice more. The type route decides the budget once, outside the per-chunk closure, and its budget refusal and synthesizer .fallback arm share one application-wide typing path. The pace moved from the ObjC bridge (whose getter Swift read back) into TextEntryTiming and is passed to the bridge, so no call site can type at a pace its own budget never charged. The test-only acknowledge window moved into the tests, and the fixture's never-asserted edits= counter is gone. fill's admitted length at --delay-ms 0 moves 214 -> 212 (honesty about the select-all record, which the old hint overpromised); the recovery hint now fits the host's 400-character diagnostic bound it previously overflowed by 60, losing its last sentence on the wire.
Size Report
Startup median (7 runs, lower is better):
|
|
Reviewed 43ea03c. I found no blocking defect. Both synthesized routes now run the step array the budget charges, and the refusal shares the Could one step executor serve both routes? It would take a plan and a post closure, slice the characters, and sleep Related, not blocking: the warmup step's pause is charged from the plan, but Smoke Tests, Repo Guards and Coverage were still running at review time, with no failures. Smoke Tests exercises this change directly. Next step: answer the executor question above. For evidence, please attach the simulator log lines from the head commit for the pace test ( |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
3 issues found across 8 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/UnitTests/RunnerTests+SynthesizedTextEntryTests.swift">
<violation number="1" location="apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/UnitTests/RunnerTests+SynthesizedTextEntryTests.swift:6">
P3: The comment's claim that `write-backs` is "the only counter this suite acts on" is inaccurate: `testSynthesizedReplacementPacesAnAppOwnedFieldAtItsAcknowledgeWindow` asserts on `after.burstEdits` (`XCTAssertGreaterThan(after.burstEdits, 1)`) and on `after.burstMilliseconds`, which are more than parse-only diagnostics, and `minimumGapMilliseconds` likewise (it appears only in a failure message, but burst counters do drive assertions). "Acts on" should be "branches on" (or the wording should distinguish the counters that gate an outcome from the ones that feed the pacing assertion).</violation>
</file>
<file name="apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/UnitTests/RunnerTests+TextEntryPolicyTests.swift">
<violation number="1" location="apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/UnitTests/RunnerTests+TextEntryPolicyTests.swift:340">
P3: The floor accepts `0.8 * delay`, so a plan undercharging up to 20% of each character's delay still passes even though the comment claims the cost "cannot come cheaper" than pace + call + delay. Use the full `delay` in the floor, or rename the tolerance so the 80% is deliberate.</violation>
</file>
<file name="apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests+TextTyping.swift">
<violation number="1" location="apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests+TextTyping.swift:203">
P3: On the `type` route the delivery budget never refuses: when `synthesizedTypeCarriesTheCommand` is false, every post is merely routed to `typeApplicationWide`, and the spaced loop keeps posting each character and sleeping `delaySeconds` between them until the whole text is typed, reporting success (failure nil, verified nil). The `TEXT_INPUT_SYNTHESIS_BUDGET_EXCEEDED` refusal exists only on the `fill` route. So for an over-budget spaced `type --delay-ms` command the text still runs to completion, one slow `app.typeText` call plus a paced sleep per character, on the same watchdogged main thread the ceiling was derived from — the exact "watchdog abandons the command with the runner still typing" failure `synthesizedDeliveryCeiling` documents it prevents. Worth confirming this is intended: if the ceiling should bound the type route too, return a budget failure here (or short-circuit the spaced loop) instead of switching channels.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
| /// field's contents never cross into the test. `write-backs` is the only counter this suite acts | ||
| /// on; the others are parsed so a fixture that stops reporting one fails here instead of silently | ||
| /// narrowing what the pacing assertion can see. |
There was a problem hiding this comment.
P3: The comment's claim that write-backs is "the only counter this suite acts on" is inaccurate: testSynthesizedReplacementPacesAnAppOwnedFieldAtItsAcknowledgeWindow asserts on after.burstEdits (XCTAssertGreaterThan(after.burstEdits, 1)) and on after.burstMilliseconds, which are more than parse-only diagnostics, and minimumGapMilliseconds likewise (it appears only in a failure message, but burst counters do drive assertions). "Acts on" should be "branches on" (or the wording should distinguish the counters that gate an outcome from the ones that feed the pacing assertion).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/UnitTests/RunnerTests+SynthesizedTextEntryTests.swift, line 6:
<comment>The comment's claim that `write-backs` is "the only counter this suite acts on" is inaccurate: `testSynthesizedReplacementPacesAnAppOwnedFieldAtItsAcknowledgeWindow` asserts on `after.burstEdits` (`XCTAssertGreaterThan(after.burstEdits, 1)`) and on `after.burstMilliseconds`, which are more than parse-only diagnostics, and `minimumGapMilliseconds` likewise (it appears only in a failure message, but burst counters do drive assertions). "Acts on" should be "branches on" (or the wording should distinguish the counters that gate an outcome from the ones that feed the pacing assertion).</comment>
<file context>
@@ -3,13 +3,16 @@ import XCTest
#if AGENT_DEVICE_RUNNER_UNIT_TESTS && os(iOS)
/// What the app-owned-value fixture reports about the edits it saw. Counts and timings only: the
- /// field's contents never cross into the test.
+ /// field's contents never cross into the test. `write-backs` is the only counter this suite acts
+ /// on; the others are parsed so a fixture that stops reporting one fails here instead of silently
+ /// narrowing what the pacing assertion can see.
</file context>
| /// field's contents never cross into the test. `write-backs` is the only counter this suite acts | |
| /// on; the others are parsed so a fixture that stops reporting one fails here instead of silently | |
| /// narrowing what the pacing assertion can see. | |
| /// field's contents never cross into the test. This suite's branch keys on `write-backs`; the | |
| /// others feed the pacing assertion, and each is required so a fixture that stops reporting one | |
| /// fails here instead of silently narrowing what the pacing assertion can see. |
| cost, | ||
| TextEntryTiming.synthesizedCharacterInterval | ||
| + TextEntryTiming.synthesizeCallOverhead | ||
| + 0.8 * delay |
There was a problem hiding this comment.
P3: The floor accepts 0.8 * delay, so a plan undercharging up to 20% of each character's delay still passes even though the comment claims the cost "cannot come cheaper" than pace + call + delay. Use the full delay in the floor, or rename the tolerance so the 80% is deliberate.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/UnitTests/RunnerTests+TextEntryPolicyTests.swift, line 340:
<comment>The floor accepts `0.8 * delay`, so a plan undercharging up to 20% of each character's delay still passes even though the comment claims the cost "cannot come cheaper" than pace + call + delay. Use the full `delay` in the floor, or rename the tolerance so the 80% is deliberate.</comment>
<file context>
@@ -259,59 +297,130 @@ extension RunnerTests {
+ cost,
+ TextEntryTiming.synthesizedCharacterInterval
+ + TextEntryTiming.synthesizeCallOverhead
+ + 0.8 * delay
+ )
+ }
</file context>
| + 0.8 * delay | |
| + delay |
| // application-wide typing: the command's own budget refused it, or XCTest's private synthesis | ||
| // is unavailable. This branch's target has no element to type into, so nothing can be read | ||
| // back afterwards: the value arrives unverified either way. | ||
| guard synthesizedTypeCarriesTheCommand else { |
There was a problem hiding this comment.
P3: On the type route the delivery budget never refuses: when synthesizedTypeCarriesTheCommand is false, every post is merely routed to typeApplicationWide, and the spaced loop keeps posting each character and sleeping delaySeconds between them until the whole text is typed, reporting success (failure nil, verified nil). The TEXT_INPUT_SYNTHESIS_BUDGET_EXCEEDED refusal exists only on the fill route. So for an over-budget spaced type --delay-ms command the text still runs to completion, one slow app.typeText call plus a paced sleep per character, on the same watchdogged main thread the ceiling was derived from — the exact "watchdog abandons the command with the runner still typing" failure synthesizedDeliveryCeiling documents it prevents. Worth confirming this is intended: if the ceiling should bound the type route too, return a budget failure here (or short-circuit the spaced loop) instead of switching channels.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests+TextTyping.swift, line 203:
<comment>On the `type` route the delivery budget never refuses: when `synthesizedTypeCarriesTheCommand` is false, every post is merely routed to `typeApplicationWide`, and the spaced loop keeps posting each character and sleeping `delaySeconds` between them until the whole text is typed, reporting success (failure nil, verified nil). The `TEXT_INPUT_SYNTHESIS_BUDGET_EXCEEDED` refusal exists only on the `fill` route. So for an over-budget spaced `type --delay-ms` command the text still runs to completion, one slow `app.typeText` call plus a paced sleep per character, on the same watchdogged main thread the ceiling was derived from — the exact "watchdog abandons the command with the runner still typing" failure `synthesizedDeliveryCeiling` documents it prevents. Worth confirming this is intended: if the ceiling should bound the type route too, return a budget failure here (or short-circuit the spaced loop) instead of switching channels.</comment>
<file context>
@@ -163,39 +196,18 @@ extension RunnerTests {
+ // application-wide typing: the command's own budget refused it, or XCTest's private synthesis
+ // is unavailable. This branch's target has no element to type into, so nothing can be read
+ // back afterwards: the value arrives unverified either way.
+ guard synthesizedTypeCarriesTheCommand else {
+ return typeApplicationWide(value, reason: "delivery-budget")
}
</file context>
|
Downstream report from a CI fleet pinning agent-device releases, before this lands. On main (#2904) Ask, since the budget model is being rewritten here anyway: make the budget a pace, never a refusal.
Happy to take this as a follow-up PR on top of this one if you prefer to keep the scope here; wanted the ask on record before the admitted length is pinned again. |
This branch has not been deployed
2 participants
Summary
Closes #2955. The delivery budget from #2904 was a parallel projection:
projectedSecondsre-derived the spaced split, the warmup peel and the warmup wait — all decided elsewhere — and under-charged even its own model, counting a select-and-type post as one synthesize call when the bridge runs two (Cmd-A record + text record).SynthesizedTextPlannow carries the posts a command makes (character counts, whether a post selects first, the wait after it). The budget charges the sum over those steps, and both synthesized routes execute that same array: the loop sleeps the plan's own pauses instead of re-derivingdelaySeconds > 0(twice more, at that). The type route decides the budget once, outside the per-chunk closure, and its budget refusal and synthesizer.fallbackarm share one application-wide typing path. The pace moved from the ObjC bridge (whose getter Swift read back through a second source of truth) intoTextEntryTimingand is passed to the bridge, so no call site can type at a pace its own budget did not charge.synthesizedAcknowledgeWindowSecondswas production dead weight; it moved into the tests. The fixture's never-assertededits=counter is gone.Numbers:
fill's admitted length at--delay-ms 0moves 214 → 212 — honesty about the select-all record the old hint overpromised (at 80 ms both models say 57). The recovery hint now fits the host's 400-char diagnostic bound it previously overflowed by 60, silently losing its last sentence viaredactDiagnosticData.Validation
Commit
43ea03ca0.pnpm check:affected --runpassed locally (xctest-selection: 0 dark tests; packaged-runner-swift ok). Local XCUITest evidence: full macOS host lane green (275 tests,TEST EXECUTE SUCCEEDED); iOS simulator lane — pace/budget/route tests all passed (testSynthesizedReplacementPacesAnAppOwnedFieldAtItsAcknowledgeWindow,testSynthesizedReplacementRefusesTextBeyondTheDeliveryBudget,testOverBudgetTypeWithoutResolvableElementTypesApplicationWide,testTypeTextReliablyPacesSynthesizedReplacementThroughProductionCaller,testBareSubmitKey…,testBareDelayedType…). tvOS build green (the fixture edits are deletions inside the existing iOS guard… verified by building tvOS).Regression evidence (mutation discipline):
synthesizeCallCount → 1→testSynthesizedPlanChargesAReplacingPostTwoSynthesizeCallsred at both asserts.peelsWarmupCharacter: false(budget ignores the peel) →testOverBudgetTypeWithoutResolvableElementTypesApplicationWidered: routesynthesized-first-responder≠xctest-application-fallback— proving the peel charge is what makes the over-budget branch reachable.Risk: admission tightened by 2 chars for
fillat delay 0 (refuses 213–214 that previously shipped and could time out mid-command).type.noneburst admission unchanged. Docs need no change;commands.md:491describes the hint's shape, which is preserved.