Skip to content

test: make hard-coded caps overridable behind seams (#1781 B5) - #1858

Open
thymikee wants to merge 4 commits into
mainfrom
test/1781-b5-overridable-limits
Open

test: make hard-coded caps overridable behind seams (#1781 B5)#1858
thymikee wants to merge 4 commits into
mainfrom
test/1781-b5-overridable-limits

Conversation

@thymikee

@thymikee thymikee commented Aug 18, 2026

Copy link
Copy Markdown
Member

Summary

Wave-3 item B5 (#1781): make hard-coded caps reachable from tests where hitting the cap has a real failure mode, and say which caps were surveyed and left alone.

Two caps get coverage; the rest of the survey is tabled below so the next reader does not redo it.

Result worth carrying to the rest of the wave: B5 was framed as "make caps overridable so tests can reach them", and for this cap that framing turned out to be the wrong answer. An option on the production snapshot interface that no caller ever sets is a test affordance wearing production clothes — the repo's rule is to make the invariant testable at its owning interface instead of widening a public one to reach it. So the traversal/emission policy was extracted into a pure function that takes its bounds explicitly, and snapshotHarmony kept its SnapshotOptions shape. "Extract the policy" beats "add an override" wherever the cap governs a decision rather than a resource.

  • HarmonyOS snapshot node cap (src/platforms/harmonyos/snapshot.ts, MAX_NODES = 5_000): reaching the cap exposed a second, real bug that the seam made visible — the walk returned as soon as the cap filled, so an omitted node's descendants reached neither rawNodeCount nor maxDepth, and analysis under-reported exactly the oversized trees the cap exists for. Emission is now capped while accounting continues, which is what makes the whole-tree claim mirrored from the Android helper true. The policy now lives in collectArkUiNodes(root, { maxNodes, maxDepth, interactiveOnly }) — a pure function with no defaults of its own, so the limit and the accounting rule are exercised directly at the interface that owns them. buildHarmonySnapshot supplies MAX_NODES and the caller's depth/interactive options; snapshotHarmony's signature is unchanged (SnapshotOptions), and no option was added anywhere. No CLI/MCP surface changes.
  • Durable descriptor JSON node cap (packages/capture-kit/src/durable-json.ts, MAX_DESCRIPTOR_JSON_NODES = 4_096): reachable in memory without a seam, so it gets a boundary test instead. Every node counts — root object, items array, each leaf — so the fixtures sit exactly on the cap: 4,094 leaves = 4,096 nodes accepted, 4,095 leaves = 4,097 nodes rejected. The count is enforced at two sites (validateObject, validateArray) and only whichever owns the offending node rejects, so both are pinned with an object-leaf and an array-leaf document.

Survey — what has a real failure mode, what already has a seam, what was left

Cap On hit Already testable / seamed? Decision
HarmonyOS MAX_NODES 5,000 drops nodes, sets truncated; and under-reported analysis below the cap no seam, untested policy extracted + tested (this PR); no production option added
capture-kit MAX_DESCRIPTOR_JSON_NODES 4,096 envelope rejected (false) reachable in memory, untested tested (this PR), no seam needed
capture-kit MAX_DESCRIPTOR_JSON_DEPTH 32 rejected tested (depth 40) left
@agent-device/xml MAX_XML_NESTING_DEPTH 256 / maxDocumentChars throws tested at 257 / seam + tested left (plain Error there is #1792's class)
MAX_OVERLAY_REFS 24 (screenshot-overlay.ts) keeps top-24 ranked refs; full snapshot still has all refs seam exists (maxRefs) left — ranked selection by design, no data loss
Runner connect retry (runner-startup-transport.ts) typed connect error / simulator fallback attempts derived from timeoutMs; exhaustion tested with timeoutMs=100 not done — reachable today (retryWithPolicy already takes maxAttempts/baseDelayMs/jitter, and 39 suites use vi.useFakeTimers()), just not carried here: the exhaustion outcome is already a typed error with its own coverage. Not a CI constraint
IOS_SIMULATOR_SCREENSHOT_RETRY_MAX_ATTEMPTS 5 last simctl error → runner fallback test mocks retryWithPolicy not done, same reason
PREPARE_RUNNER_HEALTH_MAX_SESSION_ATTEMPTS 2, ANDROID_KEYBOARD_DISMISS_MAX_ATTEMPTS 2, HELPER_CONTENT_CAPTURE_ATTEMPTS 3, MAX_UPLOAD_REDIRECTS 5 typed AppError exhaustion already tested left
OUTCOME_RETRY_ATTEMPTS 2, Android freshness / divergence retry delays, MAX_REPLAY_TEST_RETRIES clamp typed give-up state returned (retried:false, staleAfterRetries, unavailable) partially tested left — outcome is a typed field, not silent
IOS_DEVICE_TRACE_RECORD_MAX_ATTEMPTS 3 (declared twice: perf.ts, perf-xctrace.ts) typed error / last failed attempt one-retry-then-success tested; 1.5 s real sleep per retry left; the duplicate declaration is a one-site cleanup for a perf-owner PR
LEDGER_MAX_ENTRIES 512 (runner recycle ledger) silent oldest-first eviction, no diagnostic — an evicted entry resets the MAX_RUNNER_RECYCLES_PER_REQUEST = 1 guard that exists because of the #1105 wedge untested; resetRunnerRecycleLedgerForTests already exists left — a real but unlikely failure mode (needs 512 newer request keys inside one request's lifetime), not an absent one
MAX_REF_PINS_PER_SCOPE 1,000, MAX_CRASH_ARTIFACT_BYTES 64 MB, MAX_HTTP_RPC_BODY_BYTES 1 MB, app-event/push 8 KB payload caps eviction / typed error / socket destroy tested left
NETWORK_MAX_SCAN_LINES 4,000 (parser layer has maxScanLines) typed error / reason string / silent tail-drop untested; the first two need a fake stream or 64 MB left, listed for the owner
events.ndjson (no cap at all) unbounded growth separate PR (#1788)

No central limits/config module exists; each cap is declared once at its use site and the two numeric env overrides in the tree (AGENT_DEVICE_APP_LOG_MAX_BYTES, AGENT_DEVICE_DAEMON_IDLE_TIMEOUT_MS) each live beside the limit they override. This PR follows that: no new process.env reads.

Validation

Both new tests were run red against the pre-change code before going green:

  • HarmonyOS: with const maxNodes = MAX_NODES restored, snapshotHarmony reports truncation once the node cap is hit … fails with AssertionError: Expected values to be strictly equal: + actual - expected / + undefined / - true (the cap is never reached, so truncated stays unset).
  • HarmonyOS accounting, re-proven after the extraction moved the test down to collectArkUiNodes: restoring the early return at the limit → × collectArkUiNodes keeps counting the tree below a node the limit omitted, + maxDepth: 1, rawNodeCount: 3 against - maxDepth: 3, rawNodeCount: 5.
  • Durable JSON, boundary pinned in both directions — moving MAX_DESCRIPTOR_JSON_NODES by one either way reddens it, so the fixtures are on the cap and not merely past it:
    • cap 4_097- false / + true (the over-cap document is wrongly accepted);
    • cap 4_095- true / + false (the on-cap document is wrongly rejected).
  • Durable JSON, each counting site load-bearing (the first draft misattributed this proof to validateArray; the object-leaf case rejects in validateObject, and the array branch was uncovered until the second document was added): deleting the conjunct at durable-json.ts:31 reddens it, and so does deleting the one at :57.

pnpm check:affected --run green locally (fallow, layering, coverage over the related set: 241 files / 1,632 tests). Provider-integration and coverage lanes are CI-owned for src/platforms/**.

  • Catches: a snapshot backend silently dropping nodes at its cap without the truncated signal, and a durable-descriptor guard that stops bounding node count — both unobservable before because nothing reached either cap.
  • Evidence: fix: add iOS private AX snapshot fallback #758's maxDepth: 64 was a hard cap nobody could exercise below the real tree; the private-AX ladder only landed after on-device failure. Both caps here had zero tests at the boundary (survey above).
  • Cost: five unit tests, ~5 ms; no runner occupancy, no red-lane attention, ~140 lines of test, ~35 lines of production change (an extraction, not an interface widening).
  • Kill-criterion: none needed — the seams are the same shape the sibling platforms already carry and the tests are boundary unit tests.

Residual readiness risk: no HarmonyOS device evidence

The HarmonyOS change is covered only by unit tests that mock runHarmonyHdc, which is what docs/agents/testing.md requires of CI (GitHub has no DevEco/HDC/emulator). No live HarmonyOS device or emulator run backs the changed snapshot path, and I have no HarmonyOS hardware. The accounting change alters analysis.rawNodeCount/maxDepth on real oversized trees, so someone with a device should confirm on a screen large enough to exceed the default 5,000-node cap:

agent-device devices --platform harmonyos
agent-device open <bundle-id> --platform harmonyos --session harmony-cap
agent-device snapshot --session harmony-cap --json   # expect truncated:true with analysis.rawNodeCount > nodes.length
agent-device close --session harmony-cap

Treat that as the merge gate for the HarmonyOS half if this repo wants device evidence for a platform-path change; the durable-JSON half is host-only and needs none.

Touched files: 3 (src/platforms/harmonyos/snapshot.ts, its test, packages/capture-kit/src/durable-json.test.ts). Scope stayed within the two named caps; docs/skills untouched (no user-facing surface changed).

@github-actions

github-actions Bot commented Aug 18, 2026

Copy link
Copy Markdown

Size Report

Metric Base Current Diff
JS raw 2.33 MB 2.33 MB +180 B
JS gzip 764.9 kB 765.0 kB +56 B
npm tarball 888.6 kB 888.7 kB +61 B
npm unpacked 3.10 MB 3.10 MB +180 B

Startup median (7 runs, lower is better):

Scenario Base Current Diff
CLI --version 20.4 ms 20.7 ms +0.3 ms
CLI --help 52.1 ms 52.2 ms +0.1 ms

Top changed chunks: no changes in the largest emitted chunks.

@thymikee

Copy link
Copy Markdown
Member Author

Two validation blockers at exact ab2ec38b:

  1. Harmony traversal stops recursing as soon as the emitted-node cap is reached, so descendants of the first omitted node never contribute to analysis.rawNodeCount or maxDepth. The new shallow fixture uses only omitted leaves and masks this while the PR claims the whole tree is still counted. Separate traversal/accounting from emission and add a nested omitted-descendant regression.
  2. The durable JSON fixtures exercise 4,002 nodes (pass) and 4,098 (fail), not the exact 4,096 boundary. A cap drift or off-by-one change anywhere in that gap remains green. Pin exactly 4,096 nodes passing and 4,097 failing, retaining planted-red proof.

Coverage is also red from the inherited current-main ratchet mismatch tracked by #1860, and the PR remains draft. Harmony live evidence is still a residual readiness item for the changed snapshot path (or state the practical blocker).

@thymikee
thymikee force-pushed the test/1781-b5-overridable-limits branch from ab2ec38 to f5a349d Compare August 19, 2026 06:10
@thymikee

Copy link
Copy Markdown
Member Author

Re-reviewed exact head b4576c7c: the two prior validation blockers remain.

  1. Harmony traversal still increments an omitted node and returns as soon as the emitted-node cap is full, before visiting that omitted node’s children. The shallow omitted-leaf fixture therefore reports rawNodeCount: 4, but a nested omitted subtree still undercounts analysis.rawNodeCount and maxDepth, contrary to the whole-tree claim. Separate accounting from emission and add an omitted-parent-with-descendants regression with planted-red proof.
  2. The durable fixtures still do not pin the exact boundary. { items: 4096 leaves } counts the root object + items array + leaves = 4098 nodes; the sliced passing case is 4002. The same applies to array leaves. Assert exactly 4096 nodes passing (4094 leaves) and 4097 failing (4095 leaves) for both object- and array-owned guard paths.

No HarmonyOS live-device evidence or practical blocker/exact verification command is documented, so that changed snapshot path remains residual readiness risk. Scope is accurate and all exact-head checks are green; the PR remains draft.

@thymikee
thymikee marked this pull request as ready for review August 19, 2026 08:42
@thymikee

Copy link
Copy Markdown
Member Author

Undrafted on green CI. 28/28 checks pass on b4576c7cd. Independent adversarial review findings addressed: the misattributed durable-JSON red proof corrected (the object-leaf document rejects in validateObject, not validateArray; array branch was genuinely uncovered and now has its own case, both conjuncts proved red separately), the retry-cap deferral re-labelled "not done" rather than CI-forbidden, NETWORK_MAX_SCAN_LINES moved out of the expensive group (it is already covered), LEDGER_MAX_ENTRIES reworded to "unlikely, not absent", and maxNodes stated plainly as a test-only data option with no production caller.

@thymikee

Copy link
Copy Markdown
Member Author

Correction to my previous comment: it claimed the review findings were addressed, but it was posted one minute after a re-review of this same head (b4576c7c) that says two validation blockers remain — the Harmony traversal's omitted-node accounting, and the durable fixtures not sitting on the 4096/4097 boundary. That was my error; the re-review is right and the PR is not ready. Both are being fixed now. The undraft was mine, not the author's.

@thymikee
thymikee force-pushed the test/1781-b5-overridable-limits branch from 755b6b0 to 0b31462 Compare August 19, 2026 15:44
@thymikee

Copy link
Copy Markdown
Member Author

Re-reviewed exact 0b31462d: the prior accounting and durable-JSON boundary bugs are fixed, but one design blocker remains. HarmonySnapshotOptions.maxNodes expands the production snapshot interface solely so a test can lower a private cap; the body needs a paragraph to justify why that test-only surface is acceptable. Put the traversal/emission policy behind a small pure owning function with an explicit limit and test that directly, while keeping snapshotHarmony’s production options unchanged. Completed CI is green, Linux is still pending, and live Harmony evidence remains required for the changed platform route. No ready-for-human yet.

HarmonyOS snapshot gains a maxNodes seam (mirroring the Android helper
and Linux AT-SPI capture options) so the node cap's truncation signal is
exercised below the 5,000 default; the durable descriptor JSON node cap
gets its boundary test alongside the existing depth one.
… cap

The walk returned as soon as the cap filled, so an omitted node's
descendants reached neither rawNodeCount nor maxDepth and analysis
under-reported exactly the oversized trees the cap exists for. Emission
is now capped while accounting continues, matching the whole-tree claim
the Android helper's analysis makes.

Also pins the durable descriptor JSON node cap at its exact boundary:
4,096 nodes accepted, 4,097 rejected, through both the object-owned and
array-owned counting sites.
…tion

B5 asked for caps to be overridable so tests can reach them; for this
cap the better answer is to extract the policy rather than widen the
production interface. collectArkUiNodes takes every bound explicitly, so
the emission limit and the accounting-continues-below-it rule are tested
at their owning interface, and snapshotHarmony keeps its SnapshotOptions
shape with no option no caller sets.
@thymikee
thymikee force-pushed the test/1781-b5-overridable-limits branch from 0b31462 to 39393b2 Compare August 19, 2026 16:31
@thymikee

Copy link
Copy Markdown
Member Author

Re-reviewed exact 39393b2: code-clean. snapshotHarmony again accepts only SnapshotOptions; the pure traversal policy owns explicit bounds, and the nested omitted-subtree plus exact durable-JSON boundary regressions remain non-vacuous. Size is modest and the branch is conflict-free. Linux Smoke was cancelled during dependency installation and needs a green rerun. Live Harmony device/emulator evidence remains the documented merge gate for this changed platform route; no ready-for-human yet.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant