perf(dom): add snapshot-only stable planning profile#1066
Conversation
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
ⓘ You've reached your Qodo monthly free-tier limit. Reviews pause until next month — upgrade your plan to continue now, or link your paid account if you already have one. |
![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: 8c82f46745
ℹ️ 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".
| const attrMap = parseAttributes(node.attributes); | ||
| const interactive = isInteractive(tagName, attrMap); | ||
|
|
||
| if (ctx.planningProfile === 'stable' && isDecorativeMedia(tagName, attrMap, interactive)) return; |
There was a problem hiding this comment.
In planningProfile: "stable", this early return drops the entire subtree for img/picture/source/video/canvas, not just the current node. That means a non-informative <picture> wrapper can hide a child <img alt="...">, and a media-only link can lose the only labelable content from the serialized output, which degrades planning accuracy for actionable elements. The pruning logic should avoid returning before visiting children when descendants may carry meaningful attributes/text.
Useful? React with 👍 / 👎.
shaun0927
force-pushed
the
feat/998-stable-planning-profile
branch
from
8c82f46 to
09578df
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 09578df58d
ℹ️ 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".
| if (interactive) return false; | ||
| if (!['img', 'picture', 'source', 'video', 'canvas'].includes(tagName)) return false; | ||
| if (attrMap.get('alt') || attrMap.get('aria-label') || attrMap.get('role') || attrMap.get('data-testid')) return false; |
There was a problem hiding this comment.
In planningProfile: "stable", this predicate treats <video controls> (and other focusable media like tabindex="0") as decorative because interactive only checks tag/role and the allowlist here only preserves alt/aria-label/role/data-testid. As a result, actionable media elements are removed from DOM-mode output even though the feature is intended to preserve actionable controls, which can hide the only operable target on media-heavy pages.
Useful? React with 👍 / 👎.
shaun0927
force-pushed
the
feat/998-stable-planning-profile
branch
from
09578df to
37b51e0
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 37b51e065c
ℹ️ 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".
| const attrParts: string[] = []; | ||
| for (const [k, v] of attrMap) { | ||
| if (KEEP_ATTRS.has(k)) { | ||
| if (planningProfile === 'stable' && isVolatileStableAttr(k, v)) continue; |
There was a problem hiding this comment.
In planningProfile: "stable", IDs that match the volatility regex are dropped unconditionally, but related attributes like for are still emitted. On frameworks that generate IDs (e.g. ember-*/react-aria*), this breaks explicit label-to-control linkage in the serialized output (<label for="..."> no longer matches any <input id="...">), which can materially hurt planning/form understanding even though the page is otherwise unchanged.
Useful? React with 👍 / 👎.
| const attrMap = parseAttributes(node.attributes); | ||
| const interactive = isInteractive(tagName, attrMap); | ||
|
|
||
| if (ctx.planningProfile === 'stable' && isDecorativeMedia(tagName, attrMap, interactive)) return; |
There was a problem hiding this comment.
Returning early for decorative media nodes does not prevent parent-level sibling dedup from emitting summary lines (img ×N) in light/aggressive compression, so planningProfile: "stable" still leaks noisy decorative media counts when many consecutive media elements are present. This is visible in the normal read_page DOM path (default compression is light) and undermines the stated goal of omitting decorative media from planning output.
Useful? React with 👍 / 👎.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Keep planningProfile=stable snapshot pruning from removing meaningful media descendants, control-enabled media, or referenced label ids.\n\nConstraint: PR #1066 must reduce planning noise without changing live DOM behavior or default serialization.\nRejected: Disabling media pruning wholesale | targeted child traversal and signal checks preserve token savings while fixing label/media loss.\nConfidence: high\nScope-risk: narrow\nDirective: Treat stable-profile pruning as snapshot-only and preserve relationships needed for action planning.\nTested: npm ci; npm run build; npm run lint:changed; npm run lint:tier; npx jest --runInBand tests/dom/dom-planning-profile.test.ts tests/dom/dom-serializer.test.ts tests/tools/read-page.test.ts\nNot-tested: Full hosted CI matrix pending after push.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2e0f80ebc7
ℹ️ 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".
| const leafInteractive = isInteractive(leafTag, leafAttrMap); | ||
| const leafText = getDirectTextContent(leaf); | ||
| const leafLine = formatElement(leaf, leafAttrMap, '', leafText, leafInteractive); | ||
| const leafLine = formatElement(leaf, leafAttrMap, '', leafText, leafInteractive, ctx.planningProfile, ctx.referencedIds); |
There was a problem hiding this comment.
In planningProfile: "stable", decorative media is supposed to be omitted, but the container-chain collapse path emits the leaf node directly without rechecking whether that leaf is decorative. For a common structure like <div><section><img ...></section></div>, this path still serializes the <img> line, so stable output remains noisy and inconsistent with the profile’s contract. This is introduced by the new stable filtering logic because the decorative guard only runs on the current node, not on collapsed leaves.
Useful? React with 👍 / 👎.
| if (ctx.planningProfile === 'stable' && group.nodes.every(isDecorativeMediaNode)) { | ||
| for (const groupNode of group.nodes) { | ||
| serializeNode(groupNode, depth + 1, ctx); | ||
| if (ctx.truncated) return; | ||
| } |
There was a problem hiding this comment.
This stable-mode branch walks every node in a decorative-only sibling group, which bypasses existing dedup behavior and increments nodesVisited for each decorative element. On pages with large image/ad grids, that can hit the node safety cap and truncate output before later actionable controls are reached, whereas the non-stable path would summarize the group with only a few visits. The change therefore introduces a scalability regression that can drop relevant content in noisy DOMs.
Useful? React with 👍 / 👎.
Merging develop brings the shared CI test fixes into the PR branch without changing the stable-planning feature scope. Constraint: PR #1066 targets develop and its prior checks ran against a stale red baseline. Rejected: Reimplementing shared CI fixes in this branch | merging develop preserves one source of truth. Confidence: high Scope-risk: narrow Directive: Keep this branch focused on stable planning serialization after the baseline refresh. Tested: npm run build; npm run lint:changed; npm run lint:tier; npx jest --runInBand tests/dom/dom-planning-profile.test.ts tests/dom/dom-serializer.test.ts tests/tools/read-page.test.ts Not-tested: Full hosted CI matrix pending after push. Co-authored-by: OmX <omx@oh-my-codex.dev>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a28a581f8c
ℹ️ 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".
| if (ctx.planningProfile === 'stable' && group.nodes.every(isDecorativeMediaNode)) { | ||
| for (const groupNode of group.nodes) { | ||
| serializeNode(groupNode, depth + 1, ctx); | ||
| if (ctx.truncated) return; | ||
| } |
There was a problem hiding this comment.
The stable-profile branch now iterates every node in an all-decorative sibling group and calls serializeNode on each one, which bypasses the existing sibling-collapse behavior that normally samples only a few nodes plus a summary. On pages with very large decorative runs (e.g., ad/image-heavy feeds), this can drive nodesVisited to the DEFAULT_MAX_SERIALIZER_NODES cap and truncate output before later actionable elements are serialized, and it also reintroduces O(n) per-group recursion cost that dedup previously avoided.
Useful? React with 👍 / 👎.
| const referencedIds = new Set<string>(); | ||
| collectReferencedIds(root, referencedIds); |
There was a problem hiding this comment.
collectReferencedIds is executed unconditionally even when planningProfile is default, but the referenced-id set is only used by the new stable-mode ID filtering path. This adds an extra full-tree traversal and attribute parsing pass to every DOM serialization call, which is a performance regression for the default path and can materially increase latency on large documents without changing output.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d8711b7745
ℹ️ 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".
| const leafInteractive = isInteractive(leafTag, leafAttrMap); | ||
| const leafText = getDirectTextContent(leaf); | ||
| const leafLine = formatElement(leaf, leafAttrMap, '', leafText, leafInteractive); | ||
| const leafLine = formatElement(leaf, leafAttrMap, '', leafText, leafInteractive, ctx.planningProfile, ctx.referencedIds); |
There was a problem hiding this comment.
When planningProfile === "stable", container-chain collapse can still emit decorative media because the collapsed leaf is formatted directly instead of going through the early isDecorativeMedia(...) filter. In a common wrapper structure like nested divs ending in <img>/<video> without semantic attributes, this path will reintroduce the noise that stable mode is supposed to drop, increasing output size and potentially crowding out actionable nodes under the output budget.
Useful? React with 👍 / 👎.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
1 similar comment
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c3e7098b41
ℹ️ 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".
| // Common data attributes for testing and automation | ||
| 'data-cy', 'data-qa', 'data-id', 'data-value', 'data-state', | ||
| 'tabindex', | ||
| 'tabindex', 'controls', |
There was a problem hiding this comment.
controls in default DOM serialization
Adding controls to the global KEEP_ATTRS set changes planningProfile: "default" output for any <video controls> node, even though this change set is scoped as a stable-profile-only planning optimization. This introduces avoidable output drift (including larger snapshots and noisier DOM deltas) for existing default-mode consumers that did not opt into the new profile; controls should be considered only in stable-mode decorative filtering rather than globally serialized for all profiles.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e843a04417
ℹ️ 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".
| return /(?:^|[-_])(uuid|random|nonce|session|generated|ember|react-aria)[-_]?[a-z0-9]*$/i.test(value) | ||
| || /[0-9a-f]{12,}/i.test(value); |
There was a problem hiding this comment.
The stable-profile volatility check treats any id containing 12+ hex characters as volatile, so deterministic IDs like checkout-step-202605120001 or other long numeric identifiers are removed even when they are stable and useful for planning/follow-up actions. In stable mode this can strip key identifiers from actionable controls, reducing the model’s ability to disambiguate targets. Consider restricting this to stricter random/UUID-like patterns instead of any long hex substring.
Useful? React with 👍 / 👎.
| const referencedIds = new Set<string>(); | ||
| collectReferencedIds(root, referencedIds); |
There was a problem hiding this comment.
This always performs a full-tree collectReferencedIds traversal, but the collected set is only used when planningProfile === 'stable'. Because default DOM reads use the default profile, this adds an unnecessary extra O(N) pass to every serialization and can materially increase latency on large pages. Gate the prepass behind the stable profile to avoid the regression in normal read_page calls.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0b33bb5e7d
ℹ️ 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".
| attrMap.has('alt') || | ||
| attrMap.has('aria-label') || | ||
| attrMap.has('role') || | ||
| attrMap.has('data-testid') || | ||
| attrMap.has('controls') || | ||
| attrMap.has('tabindex') | ||
| ) return false; |
There was a problem hiding this comment.
In planningProfile === "stable", isDecorativeMedia treats an <img>/<video>/<canvas> as decorative unless it has one of a small set of attributes, but title is not among them. This means patterns like an icon-only link (<a><img title="Download" ...></a>) lose their only descriptive label in DOM output, because the media node is dropped while the parent link has no text; that degrades action selection and page understanding on real pages that rely on title tooltips for labeling.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7a6b437de8
ℹ️ 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".
| if (ctx.planningProfile === 'stable' && group.nodes.every(isDecorativeMediaNode)) { | ||
| for (const groupNode of group.nodes) { | ||
| serializeNode(groupNode, depth + 1, ctx); | ||
| if (ctx.truncated) return; | ||
| } |
There was a problem hiding this comment.
In planningProfile === "stable", this branch walks every node in a decorative-only sibling group, even when those nodes are intentionally omitted from output. On pages with very large runs of decorative media (the exact case this profile targets), this turns the previous dedup path into O(n) recursion, can hit the DEFAULT_MAX_SERIALIZER_NODES safety cap, and may truncate before later actionable elements are serialized. The regression only appears in stable mode with compressed traversal, but when it does, it can hide interactive content that would still be present in default mode.
Useful? React with 👍 / 👎.
Merge latest develop into PR #1066 while preserving the branch's non-brittle Tier 1 visibility smoke test. Constraint: PR #962 changed shared transport files and CI fixtures after this branch was opened.\nRejected: Restoring an exact Tier 1 count | this branch intentionally allows the Tier 1 surface to grow as core tools graduate.\nConfidence: high\nScope-risk: narrow\nDirective: Keep fixture refreshes semantically faithful to the feature branch instead of overwriting lower-bound guards with develop's exact-count snapshot.\nTested: npx jest tests/tools/console-capture-regression.test.ts tests/cross-env/cursor-verification.test.ts --runInBand --forceExit (cross-env suite may skip locally by platform guard); git diff --check for touched fixture files.\nNot-tested: Full GitHub Actions matrix after this refresh.\nCo-authored-by: OmX <omx@oh-my-codex.dev>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d9dea2cac4
ℹ️ 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".
| // Omit the decorative wrapper itself, but still inspect descendants so | ||
| // meaningful fallback labels inside <picture> or media-only links survive. | ||
| for (const child of node.children || []) { | ||
| serializeNode(child, depth, ctx); |
There was a problem hiding this comment.
In stable mode, the decorative-media branch recurses into children with the same depth value, so descendants of a dropped wrapper are treated as if they were one level shallower than they actually are. When callers set maxDepth, nodes that should be out-of-scope can still be serialized (and indented too shallowly), which breaks depth-bounded reads and can increase output unexpectedly on media-heavy pages.
Useful? React with 👍 / 👎.
Keep planningProfile=stable snapshot pruning from removing meaningful media descendants, control-enabled media, or referenced label ids.\n\nConstraint: PR #1066 must reduce planning noise without changing live DOM behavior or default serialization.\nRejected: Disabling media pruning wholesale | targeted child traversal and signal checks preserve token savings while fixing label/media loss.\nConfidence: high\nScope-risk: narrow\nDirective: Treat stable-profile pruning as snapshot-only and preserve relationships needed for action planning.\nTested: npm ci; npm run build; npm run lint:changed; npm run lint:tier; npx jest --runInBand tests/dom/dom-planning-profile.test.ts tests/dom/dom-serializer.test.ts tests/tools/read-page.test.ts\nNot-tested: Full hosted CI matrix pending after push.
Constraint: PR #1066 needed rebasing onto current develop while preserving portable fixture checks.\nRejected: Leaving conflict markers for CI to catch | TypeScript cannot parse unresolved markers.\nConfidence: high\nScope-risk: narrow\nDirective: Keep fixture comparison line-ending tolerant without changing console_capture behavior.\nTested: npm test -- --runInBand tests/dom/dom-planning-profile.test.ts tests/cross-env/cursor-verification.test.ts tests/tools/console-capture-regression.test.ts tests/cli/admin-keys.test.ts && npm run build\nNot-tested: full GitHub matrix before push
Constraint: Codex review flagged stable-profile media pruning and default serialization regressions on PR #1066.\nRejected: Traversing every decorative media sibling | large media runs can exhaust node budgets before actionable content.\nRejected: Serializing controls globally | default read_page output should not drift for non-stable consumers.\nConfidence: high\nScope-risk: narrow\nDirective: Keep stable-only pruning optimizations gated away from default DOM serialization.\nTested: npm test -- --runInBand tests/dom/dom-planning-profile.test.ts tests/dom/dom-serializer.test.ts && npm run build\nNot-tested: full GitHub matrix before push
shaun0927
force-pushed
the
feat/998-stable-planning-profile
branch
from
d9dea2c to
ab678fb
Compare
Keep planningProfile=stable snapshot pruning from removing meaningful media descendants, control-enabled media, or referenced label ids.\n\nConstraint: PR #1066 must reduce planning noise without changing live DOM behavior or default serialization.\nRejected: Disabling media pruning wholesale | targeted child traversal and signal checks preserve token savings while fixing label/media loss.\nConfidence: high\nScope-risk: narrow\nDirective: Treat stable-profile pruning as snapshot-only and preserve relationships needed for action planning.\nTested: npm ci; npm run build; npm run lint:changed; npm run lint:tier; npx jest --runInBand tests/dom/dom-planning-profile.test.ts tests/dom/dom-serializer.test.ts tests/tools/read-page.test.ts\nNot-tested: Full hosted CI matrix pending after push.
Constraint: Codex review flagged stable-profile media pruning and default serialization regressions on PR #1066.\nRejected: Traversing every decorative media sibling | large media runs can exhaust node budgets before actionable content.\nRejected: Serializing controls globally | default read_page output should not drift for non-stable consumers.\nConfidence: high\nScope-risk: narrow\nDirective: Keep stable-only pruning optimizations gated away from default DOM serialization.\nTested: npm test -- --runInBand tests/dom/dom-planning-profile.test.ts tests/dom/dom-serializer.test.ts && npm run build\nNot-tested: full GitHub matrix before push
shaun0927
force-pushed
the
feat/998-stable-planning-profile
branch
from
ab678fb to
a75eedf
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ab678fb634
ℹ️ 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".
| if (ctx.planningProfile === 'stable' && isDecorativeMedia(tagName, attrMap, interactive)) { | ||
| // Omit the decorative wrapper itself, but still inspect descendants so | ||
| // meaningful fallback labels inside <picture> or media-only links survive. | ||
| for (const child of node.children || []) { | ||
| serializeNode(child, depth + 1, ctx); |
There was a problem hiding this comment.
When planningProfile === 'stable', this branch drops the media element line and only recurses into children; however, text children are ignored later because non-element nodes return early in serializeNode. As a result, fallback labels like <video>Download demo</video> or <canvas>Chart unavailable</canvas> disappear entirely in stable output, even though default serialization would keep that text via getDirectTextContent, which can remove the only human-readable label for that content.
Useful? React with 👍 / 👎.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a75eedf055
ℹ️ 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".
| attrMap.has('aria-label') || | ||
| attrMap.has('role') || | ||
| attrMap.has('data-testid') || | ||
| attrMap.has('controls') || | ||
| attrMap.has('tabindex') |
There was a problem hiding this comment.
The new stable-profile decorative check only preserves media when alt, title, aria-label, etc. are present, but it ignores aria-labelledby/aria-describedby. In pages that label icon/media-only controls via referenced ARIA labels, this marks those nodes as decorative and drops them from stable DOM output, removing the only usable label context for planning/action selection.
Useful? React with 👍 / 👎.
| const planningProfile = (args.planningProfile as 'default' | 'stable' | undefined) ?? 'default'; | ||
| const result = await serializeDOM(page, cdpClient, { | ||
| maxDepth: depth ?? -1, | ||
| filter: filter, | ||
| interactiveOnly: filter === 'interactive', | ||
| planningProfile, |
There was a problem hiding this comment.
This introduces planningProfile for DOM reads, but the compression: "delta" return path still reconstructs output with only a [page_stats] header, so a planningProfile: "stable" request can return delta output without the [planning_profile] stable marker. That makes profile-aware consumers unable to distinguish stable vs default snapshots when deltas are emitted, leading to inconsistent downstream parsing/caching for the same tab.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: adbc3707c1
ℹ️ 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".
| if (planningProfile === 'stable') { | ||
| collectReferencedIds(root, referencedIds); |
There was a problem hiding this comment.
When planningProfile === 'stable', collectReferencedIds(root, referencedIds) performs a full recursive walk of the CDP DOM before serialization starts, but this pre-scan is not constrained by maxNodes, maxDepth, or output truncation. On very large/deep pages, that means read_page can spend most of its time in this extra traversal (or overflow the JS call stack) even though serializeNode would otherwise stop early via DEFAULT_MAX_SERIALIZER_NODES; this can turn stable mode into a timeout path on noisy production pages.
Useful? React with 👍 / 👎.
Keep planningProfile=stable snapshot pruning from removing meaningful media descendants, control-enabled media, or referenced label ids.\n\nConstraint: PR #1066 must reduce planning noise without changing live DOM behavior or default serialization.\nRejected: Disabling media pruning wholesale | targeted child traversal and signal checks preserve token savings while fixing label/media loss.\nConfidence: high\nScope-risk: narrow\nDirective: Treat stable-profile pruning as snapshot-only and preserve relationships needed for action planning.\nTested: npm ci; npm run build; npm run lint:changed; npm run lint:tier; npx jest --runInBand tests/dom/dom-planning-profile.test.ts tests/dom/dom-serializer.test.ts tests/tools/read-page.test.ts\nNot-tested: Full hosted CI matrix pending after push.
Constraint: Codex review flagged stable-profile media pruning and default serialization regressions on PR #1066.\nRejected: Traversing every decorative media sibling | large media runs can exhaust node budgets before actionable content.\nRejected: Serializing controls globally | default read_page output should not drift for non-stable consumers.\nConfidence: high\nScope-risk: narrow\nDirective: Keep stable-only pruning optimizations gated away from default DOM serialization.\nTested: npm test -- --runInBand tests/dom/dom-planning-profile.test.ts tests/dom/dom-serializer.test.ts && npm run build\nNot-tested: full GitHub matrix before push
shaun0927
force-pushed
the
feat/998-stable-planning-profile
branch
from
adbc370 to
a5127e5
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a5127e5d8d
ℹ️ 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".
| const ID_REFERENCE_ATTRS = new Set([ | ||
| 'for', | ||
| 'aria-labelledby', | ||
| 'aria-describedby', | ||
| 'aria-controls', | ||
| 'aria-owns', | ||
| ]); |
There was a problem hiding this comment.
The stable profile now drops IDs that look generated unless they appear in ID_REFERENCE_ATTRS, but this allowlist omits ARIA IDREF attributes like aria-activedescendant (and similar relationships). In widgets such as comboboxes/listboxes, the active option ID is often generated (react-aria-*), so the serializer can emit the relationship attribute while stripping the referenced element's id, breaking the reference the model needs to reason about focus/selection state.
Useful? React with 👍 / 👎.
Constraint: Planning optimization must not mutate the live page or hide actionable controls. Rejected: Global request blocking and DOM deletion | they can change task behavior and break iframe/media workflows. Confidence: medium Scope-risk: narrow Directive: Keep stable planning snapshot-only and preserve all actionable elements by default. Tested: /Users/jh0927/openchrome/node_modules/.bin/tsc -p tsconfig.json --pretty false; npx jest --config jest.config.js --runInBand tests/dom/dom-planning-profile.test.ts tests/dom/dom-serializer.test.ts; npx jest --config jest.config.js --runInBand tests/tools/read-page.test.ts Not-tested: Live noisy-page browser smoke
Keep planningProfile=stable snapshot pruning from removing meaningful media descendants, control-enabled media, or referenced label ids.\n\nConstraint: PR #1066 must reduce planning noise without changing live DOM behavior or default serialization.\nRejected: Disabling media pruning wholesale | targeted child traversal and signal checks preserve token savings while fixing label/media loss.\nConfidence: high\nScope-risk: narrow\nDirective: Treat stable-profile pruning as snapshot-only and preserve relationships needed for action planning.\nTested: npm ci; npm run build; npm run lint:changed; npm run lint:tier; npx jest --runInBand tests/dom/dom-planning-profile.test.ts tests/dom/dom-serializer.test.ts tests/tools/read-page.test.ts\nNot-tested: Full hosted CI matrix pending after push.
The health endpoint integration test should treat both code=0 and signal=SIGTERM as clean shutdown outcomes after sending SIGTERM. Constraint: CI can report SIGTERM exits as code=null even when teardown is graceful. Rejected: Extending timeouts | the failure is the accepted exit shape, not startup or shutdown latency. Confidence: high Scope-risk: narrow Directive: Avoid platform-specific assumptions for Node ChildProcess signal exit reporting. Tested: npx jest --runInBand tests/integration/health-endpoint-gating.test.ts Not-tested: Full hosted CI matrix pending after push. Co-authored-by: OmX <omx@oh-my-codex.dev>
The admin key test should verify exactly one plaintext token without failing when Jest worker console noise is captured around the in-process CLI stdout hook. Constraint: Windows CI can interleave decorated Jest console output into the captured stdout buffer. Rejected: Requiring stdout to have exactly one line | the helper already documents and handles shared stdout hook noise. Confidence: high Scope-risk: narrow Directive: Assert secret emission by token occurrence, not by total captured line count. Tested: npx jest --runInBand tests/cli/admin-keys.test.ts Not-tested: Full hosted CI matrix pending after push. Co-authored-by: OmX <omx@oh-my-codex.dev>
Constraint: Codex review flagged stable-profile media pruning and default serialization regressions on PR #1066.\nRejected: Traversing every decorative media sibling | large media runs can exhaust node budgets before actionable content.\nRejected: Serializing controls globally | default read_page output should not drift for non-stable consumers.\nConfidence: high\nScope-risk: narrow\nDirective: Keep stable-only pruning optimizations gated away from default DOM serialization.\nTested: npm test -- --runInBand tests/dom/dom-planning-profile.test.ts tests/dom/dom-serializer.test.ts && npm run build\nNot-tested: full GitHub matrix before push
Constraint: Codex review found stable-profile pruning could drop text fallback labels from media nodes.\nRejected: Re-emitting every decorative media node | that would undo the stable-profile token reduction.\nConfidence: high\nScope-risk: narrow\nDirective: Only emit pruned media when it carries direct fallback text; otherwise keep it omitted.\nTested: pending rerun after syntax repair\nNot-tested: full GitHub matrix before push
…ble profile Add aria-activedescendant, aria-flowto, and aria-details to ID_REFERENCE_ATTRS so listbox/combobox patterns retain their referenced volatile IDs in stable mode. Also fix formatElement call-site argument order after rebase conflict resolution. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
shaun0927
force-pushed
the
feat/998-stable-planning-profile
branch
from
a5127e5 to
d8b9f7a
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d8b9f7aedd
ℹ️ 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".
| // Omit decorative media wrappers without fallback text, but still inspect | ||
| // descendants so meaningful labels inside <picture> survive. | ||
| for (const child of node.children || []) { | ||
| serializeNode(child, depth + 1, ctx); |
There was a problem hiding this comment.
Pass the child path into this recursive call; otherwise it falls back to the default path = 'd', so descendants of dropped decorative nodes lose their true DOM path context. In stable mode with filter: 'interactive', non-native interactive descendants (detected via customInteractiveHints) under <video>/<canvas> fallback content can be misclassified as non-interactive and omitted, because hint lookup is path-based and no longer matches.
Useful? React with 👍 / 👎.
# Conflicts: # src/tools/read-page.ts # tests/cli/admin-keys.test.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cab5476196
ℹ️ 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".
| if (fallbackText) { | ||
| const line = formatElement(node, attrMap, indent, fallbackText, interactive, customHints, ctx.planningProfile, ctx.referencedIds); | ||
| if (!appendBoundedLine(ctx, line + '\n')) return; | ||
| ctx.emittedBackendNodeIds.add(node.backendNodeId); |
There was a problem hiding this comment.
In planningProfile === 'stable', decorative media with fallback text is emitted before the normal interactiveOnly gate runs, so filter: 'interactive' responses can include non-interactive <video>/<canvas> lines whenever they contain direct text. This is a behavioral regression from the previous path (which only emitted when interactive) and can pollute interactive-only planning output with non-actionable nodes.
Useful? React with 👍 / 👎.
| if (ctx.planningProfile === 'stable' && group.nodes.every(isDecorativeMediaNode)) { | ||
| // A purely decorative media run contributes no planning signal. Skip it | ||
| // as a group instead of visiting every omitted leaf and exhausting the | ||
| // serializer node budget on ad/image-heavy pages. | ||
| continue; |
There was a problem hiding this comment.
The stable-mode sibling fast-path skips an entire group when group.nodes.every(isDecorativeMediaNode), but that predicate only treats element descendants as meaningful. As a result, consecutive fallback-text media (for example several <video>Download</video> siblings) are classified as decorative and removed wholesale under light/aggressive compression, even though single-node traversal now preserves that fallback text. This introduces content loss that depends on sibling count.
Useful? React with 👍 / 👎.
1 participant
Progress / Review status
Auto-refreshed 2026-05-13 — owner comments cleaned up to reduce review noise.
feat/998-stable-planning-profile→developd8b9f7a— fix(dom): preserve volatile IDs referenced by ARIA IDREF attrs in sta…Owner comment cleanup: 12 issue + 0 inline review comments deleted. Outstanding feedback from automated/external reviewers above is unchanged.
Summary
planningProfile: "stable"for DOM serialization/read_page planning pathsCloses #998.
Alignment / scope review
This is intentionally snapshot-only. It does not block network requests, delete DOM nodes, change click behavior, add dependencies, or alter default
read_pageoutput.Verification
/Users/jh0927/openchrome/node_modules/.bin/tsc -p tsconfig.json --pretty falsenpx jest --config jest.config.js --runInBand tests/dom/dom-planning-profile.test.ts tests/dom/dom-serializer.test.tsnpx jest --config jest.config.js --runInBand tests/tools/read-page.test.tsPost-merge smoke
Run the issue's noisy-fixture OpenChrome MCP flow to compare default vs stable DOM bytes and verify
interactstill works after stable planning.