Skip to content

🤖 refactor: collapse PTC to exclusive-only (single PTC experiment, RLM sub-experiment) - #3963

Open
ThomasK33 wants to merge 19 commits into
mainfrom
ptc-mode-w7we
Open

🤖 refactor: collapse PTC to exclusive-only (single PTC experiment, RLM sub-experiment)#3963
ThomasK33 wants to merge 19 commits into
mainfrom
ptc-mode-w7we

Conversation

@ThomasK33

@ThomasK33 ThomasK33 commented Aug 25, 2026

Copy link
Copy Markdown
Member

Summary

Collapses PTC to exclusive-only: the single programmatic-tool-calling experiment now always replaces the standard toolset with a sandboxed code_execution tool (bridgeable tools hidden; non-bridgeable tools and mcp_prompt_get stay model-visible). The programmatic-tool-calling-exclusive experiment ID and the programmaticToolCallingExclusive flag are deleted everywhere, and RLM remains a sub-experiment gated simply on rlm && ptc.

Background

Past evals showed supplement-mode PTC (code_execution alongside the flat tools) measured ~2x tokens/cost versus both PTC-off and PTC-exclusive: flat schemas plus bridge type definitions shipped on every request while models still took the flat path. With supplement mode removed there is no reason for two experiments, so the existing programmatic-tool-calling ID keeps its meaning as "PTC on" and users who had it enabled seamlessly upgrade to the (better-measuring) exclusive posture.

Implementation

  • toolAssembly.ts: the PTC branch is unconditionally exclusive; the supplement else branch and the exclusiveActive predicate are gone. RLM stays parent-gated by construction (the flag is only read inside the PTC branch).
  • aiService.ts: deleted rebuildCodeExecutionAfterAssembleHook plus both call sites. It existed solely for supplement mode, where request.assemble middleware could edit hook-visible bridgeable tools out from under the pre-hook code_execution bridge; in exclusive mode bridgeable tools are never in the hook-visible record. The now-orphaned retarget/reconcile helper chain (retargetCodeExecution, reconcileHookReplacedCodeExecution, retargetCodeExecutionTool + the late-bound retarget WeakMap in code_execution.ts) is removed with it.
  • RLM gating: only the two central predicates changed (isRlmModeEnabled, resolveSlashCommandExperimentValue) — a prior audit found zero raw RLM activation sites outside them and the PTC-gated assembly branch, so every RLM surface (task-family messaging, compaction keep-recent floor, /refine, branch summaries, persistent mounts, refinement_rollback) inherits the rlm && ptc gate.
  • Compatibility: instead of dropping the removed exclusive flag, every persistence layer that crosses build versions now aliases it — backend feature_flags.json, renderer localStorage, and persisted taskExperiments all read a legacy exclusive true as merged PTC and mirror PTC back onto the legacy key on write, so upgrade preserves the exclusive posture and downgrade never falls back to ~2x supplement mode (see review rounds below). In-sync IPC schemas still just strip the unknown key.

Validation

  • Grep gates: zero references to the exclusive ID/flag outside three intentional stale-payload tests; RLM raw-activation audit re-verified post-rebase.
  • Dogfooded in an isolated dev-server sandbox via agent-browser (screenshots + webm in the workspace transcript):
    • Settings → Experiments shows exactly one PTC row; RLM nests under it when enabled and disappears when PTC is off; verified at 375px mobile width.
    • With { ptc: true, rlm: false }, devtools.jsonl shows the provider request toolset todo_read, todo_write, web_fetch, web_search, code_executionbash/file_read/file_edit_* absent from the model-visible set but declared as bridged xum.* functions; no kernel surfaces.
    • Adding rlm: true surfaces refinement_rollback and the kernel-first description preamble.
    • Injected "programmatic-tool-calling-exclusive": true into feature_flags.json and restarted: clean boot, key ignored and self-healed out on the next write, surviving flags intact.
  • 4 local test failures in taskService/workspaceService reproduce identically on a clean-HEAD probe worktree (known local-Bun-version issues, unrelated).

Review round 1 (Codex) — hardening additions

  • Fail closed on PTC assembly failure: code_execution creation errors now fail the send for all PTC (previously only RLM); silently degrading to flat tools would change semantics while the run is recorded as PTC.
  • Disable-all policies win: when the tool policy leaves no tools (auto-compaction's .* disable rule), code_execution is no longer synthesized.
  • Policy-required tools stay top-level: require rules gate run completion on top-level toolResults, so required tools are retained in the model-visible set instead of being bridged away.
  • memory/advisor + attach_file/desktop_screenshot are non-bridgeable: system-prompt context (memory index/hot set, advisor guidance) keys off top-level presence, and media-producing built-ins stay top-level for guaranteed model visibility in both classic and kernel modes.
  • Nested persistence extractors: extractEditedFileDiffs/extractEditedFilePaths and extractLoadedSkillSnapshotsFromMessages now consume successful nested PTC records (mirroring extractReadFiles).
  • Legacy flag compatibility alias: a persisted programmatic-tool-calling-exclusive: true now maps onto PTC at read time (upgrade keeps the posture), and an enabled PTC mirrors the legacy key on write (downgrade runs the old exclusive posture instead of ~2x supplement mode).

Review round 2 (Codex) — compatibility + media hardening

  • taskExperiments legacy alias: persisted workspace configs parse programmaticToolCallingExclusive: true onto programmaticToolCalling (schema preprocess), so tasks stamped by older builds keep exclusive PTC — and their rlm flag stays effective — on restart-safe resumption.
  • Renderer localStorage sync: every PTC toggle rewrites the legacy experiment:programmatic-tool-calling-exclusive key (a downgraded renderer treats it as an explicit override that beats the mirrored backend value), and renderer reads alias a stored legacy true onto PTC, matching the backend alias semantics.
  • Nested media extraction instead of elision: the bridge-side media elision from round 1 is replaced — bridged results pass through intact, and extractAttachmentsFromToolOutput now traverses code_execution toolCalls records at request time, turning nested bridged-MCP media into placeholder + synthetic user file parts exactly like top-level tool media (shared by the main, mid-stream, and replay request builders). Kernel-compacted records drop result contents by design; the guest still holds the data.
  • Newest-first nested edit paths: extractEditedFilePaths traverses nested batches in reverse so a large batch keeps its latest edits under MAX_EDITED_FILES.

Review round 3 (Codex) — runtime paths + duplication hardening

  • Runtime config-loader alias: normalizePersistedWorkspace (the actual loadConfigOrDefault path, which never runs the Zod preprocess) aliases a persisted legacy exclusive flag onto programmaticToolCalling, retaining the legacy key for downgrade.
  • Persisted-state helpers: the renderer legacy-key mirror routes through updatePersistedState/readPersistedState, joining the shared write-listener/subscriber notification path.
  • RLM persistence records: kernel record compaction exempts agent_skill_read and file_edit_* records (result kept, args/error bounded like mux.load records) so loaded-skill snapshots and edited-file diffs survive compaction under PTC+RLM; legacy result-less records still degrade to path-only tracking.
  • Outer-result media: the code_execution media extractor also rewrites the guest's return value and dedupes identical media between record and return value into one attachment.
  • No duplicated bridge dispatch: tools promoted to the model-visible set (policy-required, mcp_prompt_get) are excluded from the ToolBridge, so request.assemble hooks always see the only dispatch path — preserving the premise that justified deleting the assemble-hook rebuild machinery.

Review round 4 (Codex) — kernel capture bounding + ordering

  • Creation-time exemption: KernelRecordBounds.resultExempt (shared predicate isKernelRecordResultExempt) exempts persistence-critical results at record capture, closing the gap where >16KB file_edit_* diffs / agent_skill_read snapshots were __kernelBounded before the compaction exemption could see them.
  • RLM-safe MCP media: the predicate also exempts media-bearing content containers through capture and compaction, so bridged MCP screenshots reach request-time attachment extraction in kernel mode without relying on the static non-bridgeable list.
  • Part-level newest-first: extractEditedFilePaths walks a message's parts backward (mirroring extractReadFilePaths) so later executions' edits win the MAX_EDITED_FILES cap.

Review round 5 (Codex) — downgrade snapshots + media scope

  • Task-snapshot downgrade mirror: toPersistedTaskExperiments stamps the legacy exclusive flag alongside enabled PTC in persisted taskExperiments (schema now declares the field), so downgraded builds resume tasks in exclusive posture.
  • Null-safe nested records: collectNestedEditRecords skips malformed (null/primitive) nested results instead of throwing — one corrupt history row can no longer wedge compaction flows.
  • Extractable-only media exemption: the kernel media exemption requires a supported attachment type, and request-time extraction replaces unsupported media (audio/blobs) with bounded placeholders, closing the raw-base64 path for media the model can never consume.

Review round 6 (Codex) — capture sanitizer + policy/CLI compat

  • Mixed-media capture sanitizer: the kernel capture exemption became a retain-transform (KernelRecordBounds.captureRetained): mixed containers keep extractable images/PDFs but unsupported parts (audio/blobs) are replaced with bounded placeholders before the record is retained/persisted.
  • Allowlist probe: the no-tools contract check probes the synthesized code_execution name (applyToolPolicyToNames), so [disable .*, enable code_execution] allowlists keep the exclusive entry point.
  • CLI compat alias: xum run/workflow -e programmatic-tool-calling-exclusive maps onto the merged PTC flag instead of erroring.
  • Console media redaction: request-time extraction rewrites container-shaped consoleOutput args (the console.log(image) debugging path), deduped into a single attachment.

Review round 7 (Codex) — retry snapshots + capture budgets

  • Startup-retry snapshot compat: shared aliasLegacyPtcExclusive/withLegacyPtcExclusiveMirror helpers now cover retry/preserved send options too (schema preprocess + declared mirror field + raw-JSON read-site alias + write mirrors), so interrupted turns resume in the exclusive posture across upgrades and downgrades.
  • Capture-bounded persistence shapes: fileedit*/agent_skill_read results are reduced at capture to the extractor-consumed shape with the diff/snapshot 50k cap enforced (keeping the downstream truncation signal), preventing large-edit loops from persisting unbounded ui_only diffs.
  • Aggregate media budget: retained kernel media is capped at 3 MiB per record; over-budget parts become bounded placeholders before events/records are emitted.

Review round 8 (Codex) — capture-bounding hardening

  • Bounded-args path retention (P1): when kernel args bounding replaces a file_edit_* record's args with a __kernelBounded marker (>2 KiB inserts), the validated path is merged back onto the marker so post-compaction diff preservation and edited-file tracking still attribute the edit.
  • Serialized media budget: the aggregate media budget now charges each retained part's full serialized size (metadata included — mimeType-stuffing with empty data no longer rides free), caps part count, and bounds placeholder labels.
  • Skill body truncation: oversized agent_skill_read packages keep a schema-valid skill with a truncated body (mirroring createLoadedSkillSnapshot) instead of being dropped to a marker.
  • Hunk-boundary diff truncation: oversized diffs are bounded at hunk boundaries (parseable, applicable prefix) with a diffTruncated flag propagated to FileEditDiff.truncated, so combined diffs stay composable and honest.
  • Classic-mode media budget: a new mode-independent capture sanitizer budgets media containers in default (non-RLM) exclusive PTC records too; the guest still receives full values.

Review round 9 (Codex) — persisted-copy hardening

  • Classic outer-result sanitization: classic executions budget the guest's returned value through the same capture sanitizer before it persists (return xum.<mediaTool>() can no longer write an unbudgeted multi-image row); console args are sanitized at capture in both modes so streamed event copies are bounded too. Kernel mode's outer result is deliberately excluded (vars-handle offloading stores full fidelity for the guest).
  • UTF-8 byte budget: the aggregate media budget charges serialized UTF-8 bytes, not UTF-16 code units, closing the multibyte-metadata (~3x) bypass. Persistence-critical diff/skill caps intentionally remain char caps, matching MAX_FILE_CONTENT_SIZE semantics repo-wide.

Review round 10 (Codex)

  • Legacy alias in follow-up dispatch (P1): dispatchPendingFollowUp reads preserved experiments through aliasLegacyPtcExclusive, matching the startup-retry path, so pre-rename persisted follow-ups keep PTC enabled.
  • Deep media sanitization: the capture sanitizer walks the whole returned/console value graph (memoized, cycle-safe, fail-closed depth cap) with one shared aggregate budget per value — wrapped containers can no longer bypass the bound or multiply it.
  • Execution-wide retained budget: retained kernel results (media containers, persistence-critical records) are charged against a 12 MiB per-execution budget; on exhaustion they degrade to honest-size markers, bounding host memory and persisted row size under retained-call loops.

Review round 11 (Codex)

  • Media-type validation: isSupportedAttachmentMediaType requires a well-formed type/subtype within a 100-char bound (not just an image/ prefix), so junk MIME strings fail validation at capture retention, request extraction, and provider output sanitization alike; placeholder builders additionally bound interpolated media-type/filename labels for history persisted by earlier builds.

Review round 16 (Codex)

  • Deep JSON preserved: the extraction wrapper walk stops at the depth cap without substituting placeholders — over-depth replacement applies only to tool-output-shaped chains, so legitimate media-free deep JSON is never truncated.
  • Positive success bits: result-less nested edit AND read records require an explicit ok === true; malformed rows can no longer advertise never-applied edits or never-read paths in crash-safe tracking.

Review round 15 (Codex)

  • Wrapper deep-walk in request-time extraction (P1): provider-copy media extraction traverses arbitrary wrapper objects/arrays (shared depth cap, over-deep → bounded placeholder), so containers wrapped in outer results or console args are rewritten into deduplicated attachments instead of shipping as raw JSON alongside the attachment.

Review round 14 (Codex)

  • String-only diff guards (P1): edit extractors admit diff values from persisted history only when they are strings (nested records and direct parts alike), so corrupt rows degrade to path-only tracking instead of throwing in parsePatch on every compaction/recovery pass.

Review round 13 (Codex)

  • Over-depth subtree replacement (P1): the extraction depth guard replaces malformed over-deep tool-record chains with a bounded placeholder in the provider copy (instead of retaining them), so leaf payloads in corrupt rows cannot repeatedly trigger context-limit failures; persisted history is never mutated.

Review round 12 (Codex)

  • Extraction recursion bound (P1): request-time attachment extraction depth-caps its walk over nested tool records (corrupt deep history rows degrade to no extraction instead of stack-overflowing every request).
  • Marker attribution after budget overflow: capture-bounded __kernelBounded results compact to the normal {ok, bytes} summary (keeping edit path attribution), and a boolean success bit is preserved through the marker so failed edits never misreport ok:true.

Risks

Low-to-moderate, contained to the opt-in PTC experiment (off by default). The main behavior change is intentional: users with PTC (supplement) enabled now get the exclusive toolset. The deleted assemble-hook rebuild only ever ran in supplement mode, so middleware/tool-policy interactions in exclusive mode are unchanged. Users who had only the exclusive toggle enabled keep their posture via the read-side aliases (backend, renderer, and taskExperiments), so no re-enable step remains.


📋 Implementation Plan

Collapse PTC to exclusive-only: single PTC experiment + RLM sub-experiment

Goal

Past evals show supplement-mode PTC (code_execution alongside normal tools) performs worse than both PTC-off and PTC-exclusive. Remove supplement mode entirely:

  • One PTC experiment (programmatic-tool-calling) that always activates today's exclusive posture (bridgeable tools hidden; code_execution + non-bridgeable tools + mcp_prompt_get model-visible).
  • Delete the programmatic-tool-calling-exclusive experiment ID and the programmaticToolCallingExclusive flag everywhere.
  • RLM stays a sub-experiment nested under PTC (gating becomes simply rlm && ptc).

Acceptance criteria

  1. Exactly one PTC experiment is visible in Settings → Experiments (no "PTC Exclusive Mode" row).
  2. { programmaticToolCalling: true, rlm: false } yields exclusive tool visibility: code_execution present; bridgeable tools (bash, file_read, file_edit_*, task, …) absent from the model-visible set; non-bridgeable tools (todo_write, ask_user_question, …) and mcp_prompt_get remain.
  3. { rlm: true } with PTC off is completely inert — no RLM-only surface activates: task-family messaging (task_message_parent/task_message_sibling), compaction keep-recent floor, /refine (visibility AND backend RefineService), abandoned-branch summaries, persistent sandbox mounts, kernel preambles/xum.load, and refinement_rollback all stay off.
  4. { programmaticToolCalling: true, rlm: true } enables kernel/RLM surfaces (persistent mount, refinement_rollback, /refine).
  5. Zero product-code references to programmaticToolCallingExclusive / PROGRAMMATIC_TOOL_CALLING_EXCLUSIVE / programmatic-tool-calling-exclusive remain (grep gate below).
  6. Stale legacy payloads are ignored, never rejected (verified: readOverridesFile skips keys not in EXPERIMENTS; ExperimentsSchema and taskExperiments are plain z.object — Zod strips unknown keys, no .strict() anywhere in those schemas).

Key decisions (assumptions)

  • Keep the existing programmatic-tool-calling ID. Users who had PTC (supplement) enabled seamlessly upgrade to exclusive — exactly the desired behavior.
  • No migration for exclusive-only users. Both persistence layers ignore unknown experiment keys on read (verified: readOverridesFile in src/node/services/experimentsService.ts filters against EXPERIMENTS; Zod strips unknown sendOptions.experiments keys since the schemas are non-strict), so stale programmatic-tool-calling-exclusive entries in ~/.xum/feature_flags.json / localStorage are inert. A user who had only the exclusive toggle on re-enables PTC once in Settings. Per AGENTS.md, migrations are skipped when breakage is tightly scoped — this is an opt-in experiment, off by default.
  • Downgrade is friction-free: old builds read programmatic-tool-calling: true and run supplement mode (their own valid behavior).
  • Do not retain rebuildCodeExecutionAfterAssembleHook behind the exclusive path. It exists solely for supplement mode where hook-visible tools and the bridge coexist; exclusive mode already skips it today, so deletion does not change exclusive semantics (advisor-confirmed).

Changes

1. Experiment definitions — src/common/constants/experiments.ts

  • Delete EXPERIMENT_IDS.PROGRAMMATIC_TOOL_CALLING_EXCLUSIVE (line 10) and its EXPERIMENTS entry (lines 62–68).
  • Update PTC description (currently "Enable code_execution tool for multi-tool workflows...") to state it replaces the standard toolset: e.g. "Replace the standard toolset with a sandboxed code_execution tool; bridged tools are called as xum.(...) from JS".
  • Trim the RLM description's "Implies PTC Exclusive posture; supplement mode is not supported" tail to "Requires Programmatic Tool Calling."

2. Tool assembly — src/node/services/toolAssembly.ts

  • Options type (line ~134) and resolveBackendGatedPtcExperiments (lines ~186–188): drop the exclusive field/backfill; keep programmaticToolCalling + rlm.
  • Core logic (lines ~240–324):
    • Gate stays if (experiments?.programmaticToolCalling) — RLM alone still does nothing (parent-gated, unchanged).
    • Delete exclusiveActive; the branch is always exclusive now. Delete the supplement else branch (applyToolPolicy({ ...policyFilteredTools, code_execution }, ...)).
  • Update the module doc comment (line ~200 "Supplement mode: adds codeexecution alongside...") and the RLM-is-exclusive-only comment (line ~236) — keep the "supplement measured ~2x tokens/cost" rationale as the _why this mode was removed.

3. AI service — src/node/services/aiService.ts

  • Delete rebuildCodeExecutionAfterAssembleHook (method + doc comment, ~lines 1080–1240) — it exists solely because supplement mode exposed a pre-hook bridge alongside hook-visible tools. Exclusive mode is unaffected by design (bridgeable tools aren't in the hook-visible record).
  • Delete both call sites + their guard blocks (lines ~3008–3028 and ~3733–3749).
  • Simplify ptcEnabled predicate (line ~2923–2925) to experiments?.programmaticToolCalling === true.
  • Remove imports/params that become unused (typecheck/lint will flag them).

4. RLM gating (full parent-gating audit completed)

An exhaustive audit of every runtime consumer of the RLM flag (experiments?.rlm, EXPERIMENT_IDS.RLM, isRlmModeEnabled, rlmActive) found zero raw sites: every RLM surface is parent-gated either through the two central predicates below or by being lexically inside toolAssembly.ts's PTC-gated branch (line 242). Audited surfaces: task-family messaging (aiService.ts:2739 via isRlmModeEnabled(taskExperiments)), compaction keep-recent floor (agentSession.ts:4202 via isRlmModeEnabled), RefineService.enabled() (refineService.ts:345), abandoned-branch summaries (branchSummary.ts:702), persistent mounts + kernel preambles + refinement_rollback (all inside the PTC branch of toolAssembly.ts), sandboxHostService (no experiment checks at all — pure mechanism invoked only from the gated branch), task/workflow experiment inheritance (transport-only; evaluated via isRlmModeEnabled at consumption), CLI builders (don't expose rlm at all).

Therefore only the two central predicates need changing — every other surface inherits:

  • src/node/services/branchSummary.ts: drop programmaticToolCallingExclusive from RlmExperimentFlags; isRlmModeEnabledrlm && ptc.
  • src/browser/utils/slashCommands/experimentVisibility.ts: snapshot type + RLM case → snapshot.rlm === true && snapshot.programmaticToolCalling === true.

Existing parent-gating tests that must stay green (with exclusive permutations removed, no coverage loss): branchSummary.test.ts isRlmModeEnabled cases, suggestions.test.ts "requires a PTC parent flag for rlm-mode", refineService.test.ts "refuses when RLM is on but no PTC parent flag" (line ~327), toolAssembly.test.ts "refinement_rollback is exposed only with rlm on" (ptcOff case asserts rlm: true alone yields no code_execution/refinement_rollback), agentSession.autoCompaction.test.ts "stamps ... keep-recent tail only when RLM is on" (rlm-without-PTC → no stamp).

5. Schemas / shared types (frontend↔backend IPC is always in sync — remove outright)

  • src/common/orpc/schemas/stream.ts ExperimentsSchema (line 743)
  • src/common/schemas/project.ts taskExperiments (line 183)
  • src/common/utils/tools/tools.ts ToolConfiguration.experiments (line 283)
  • src/node/services/taskService.ts TaskOptions.experiments (line 307)
  • src/node/services/workflows/WorkflowTaskServiceAdapter.ts (line 33)
  • src/browser/utils/messages/buildSendMessageOptions.ts ExperimentValues (line 8)

6. Frontend surfaces

  • ExperimentsSection.tsx: remove ptcExclusiveEnabled subscription (lines 698–700) and the second RLM nesting block + its dedup comment (lines ~811–820). Single PTC row keeps PTC_SUB_EXPERIMENT_IDS = [RLM] nested when enabled.
  • ChatInput/index.tsx (lines 330, 1755, 1810), CommandPalette.tsx (lines 73, 309), useSendMessageOptions.ts (lines 58–59, 83), sendOptions.ts (lines 96–97): remove the exclusive subscription/field pass-through.

7. CLI + eval script

  • src/cli/run.ts (line 290), src/cli/workflow.ts (lines 230–231): drop the exclusive entry from buildExperimentsObject. Unknown --experiment values are already ignored by the includes pattern — no extra validation needed.
  • scripts/rlm-eval/scenarios.ts: drop the optional programmaticToolCallingExclusive field (line 32); re-describe scenarios so future evals don't imply supplement PTC still exists: the ptc-only config ({ programmaticToolCalling: true, rlm: false }) now measures exclusive PTC without the kernel — note that in its comment; trim the stale "explicit exclusive flag is redundant but harmless" comment above rlm-excl (lines 243–245). flat-bash baseline is unaffected.

8. Comment hygiene (small, optional but cheap)

Reword "supplement-mode contract"/"byte-identical supplement contract" mentions to "non-RLM inline-results contract" so the term doesn't dangle: src/node/services/ptc/types.ts:61, ptc/toolBridge.ts:215, ptc/runtime.ts:91, src/common/utils/messages/extractReadFiles.ts:23.

9. Tests

  • src/node/services/toolAssembly.test.ts: replace "PTC only: supplement set, no kernel surfaces" with "PTC only: exclusive narrowed set, no kernel surfaces"; rewrite exclusive-flag tests (mcp_prompt_get visibility, grants ceiling, kernel permutations) to use programmaticToolCalling; update resolveBackendGatedPtcExperiments tests.
  • src/node/services/branchSummary.test.ts: drop exclusive-flag permutations from isRlmModeEnabled tests.
  • src/browser/utils/slashCommands/suggestions.test.ts: "requires a PTC parent flag for rlm-mode" → single-parent form.
  • src/node/services/refinement/refineService.test.ts: update flag payloads that set exclusive.
  • Sweep remaining test payloads (workspaceService.test.ts, agentSession.*.test.ts, taskService.test.ts, stream.test.ts) — most already use { programmaticToolCalling: true, rlm: true } and need no change; typecheck flags any stragglers.
  • ExperimentsSection.test.tsx / .stories.tsx: nesting-under-PTC assertions should pass unchanged; verify.

Net LoC estimate (product code only)

≈ −230 LoC (deletions dominate: ~−170 in aiService.ts, ~−15 toolAssembly.ts, ~−10 experiment definition, ~−25 frontend, ~−10 schemas/types/CLI; +~10 reworded copy/comments). Tests shrink additionally.

Validation

  1. make typecheck first — deleting the field from shared types drives an exhaustive compile-error sweep of any missed references.
  2. Targeted tests: bun test src/node/services/toolAssembly.test.ts src/node/services/branchSummary.test.ts src/browser/utils/slashCommands/suggestions.test.ts src/node/services/refinement/refineService.test.ts src/node/services/agentSession.autoCompaction.test.ts src/node/services/experimentsService.test.ts src/browser/features/Settings/Sections/ExperimentsSection.test.tsx plus wherever the schema parse/strip tests from step 3 land (e.g. the stream/project schema test files).
  3. Stale-payload tests (guard the no-migration decision): add/extend small unit tests asserting that (a) a feature_flags.json overrides map containing "programmatic-tool-calling-exclusive": true is ignored by readOverridesFile (experimentsService.test.ts already covers unknown-key filtering — extend only if no such case exists), and (b) ExperimentsSchema.parse / taskExperiments parse of a payload containing programmaticToolCallingExclusive succeeds and strips the key.
  4. Grep gates:
    • grep -rn "programmaticToolCallingExclusive\|PROGRAMMATIC_TOOL_CALLING_EXCLUSIVE\|programmatic-tool-calling-exclusive" src scripts tests returns nothing except the intentional stale-payload legacy tests from step 3 (raw string literals only, no symbol references).
    • RLM raw-check gate: grep -rn "experiments?.rlm\|EXPERIMENT_IDS.RLM\|rlmActive" src — every hit must be one of: (a) the central predicates (isRlmModeEnabled / resolveSlashCommandExperimentValue) or callers passing flags into them, (b) inside toolAssembly's PTC-gated branch, (c) transport/schema packaging (sendOptions/taskExperiments/ExperimentsSchema fields), (d) the experiment definition in experiments.ts, (e) Settings nesting / UI subscriptions that only feed the central resolver (ExperimentsSection, ChatInput, CommandPalette), or (f) tests. Any new raw activation site fails review.
  5. make static-check (use MUX_ESLINT_CONCURRENCY=1 if memory-constrained).

Dogfooding (quality gate before declaring done)

Using the dev-server-sandbox skill (isolated XUM_ROOT + free port) + agent-browser:

  1. Settings UI: open Settings → Experiments. Verify exactly one PTC row (no "PTC Exclusive Mode"), toggle it on, verify RLM appears nested beneath it. Screenshot both states (off / on-with-nested-RLM) and attach_file them.
  2. Mobile width: repeat the Settings check at ~375px viewport (agent-browser set viewport) — screenshot to confirm the experiment rows don't overflow (AGENTS.md mobile-width rule).
  3. Video evidence: record the flow (agent-browser video recording) covering the Settings toggle + one scratch-workspace message send, and attach_file the recording.
  4. Exclusive toolset (PTC on, RLM off): enable API Debug Logs, send a trivial message in a scratch workspace, then read <XUM_ROOT>/sessions/<workspace>/devtools.jsonl and assert the provider request's tool list contains code_execution but no bash/file_read/file_edit_* (non-bridgeable tools like todo_write may remain).
  5. RLM nesting: enable RLM too, send another message, confirm kernel-first posture (e.g. refinement_rollback present / kernel preamble) in devtools.jsonl.
  6. Stale-flag resilience: write "programmatic-tool-calling-exclusive": true into the sandbox's feature_flags.json overrides, restart the dev server, confirm the app loads cleanly and the key is ignored (no crash, PTC off unless the surviving flag is on).

Attach screenshots (steps 1–2), the video (step 3), and include the devtools.jsonl tool-list evidence in the summary.


Generated with xum • Model: anthropic:claude-fable-5 • Thinking: xhigh • Cost: $175.92

…iment

Past evals showed supplement-mode PTC (code_execution alongside normal
tools) measured ~2x tokens/cost vs both PTC-off and exclusive. Remove
supplement mode entirely:

- The programmatic-tool-calling experiment now always activates the
  exclusive posture (bridgeable tools hidden; code_execution +
  non-bridgeable tools + mcp_prompt_get model-visible).
- Delete the programmatic-tool-calling-exclusive experiment ID and the
  programmaticToolCallingExclusive flag everywhere (schemas, IPC types,
  frontend subscriptions, CLI experiment builders, eval scenarios).
- RLM gating simplifies to rlm && ptc via the central predicates
  (isRlmModeEnabled, resolveSlashCommandExperimentValue); RLM alone
  stays fully inert.
- Delete rebuildCodeExecutionAfterAssembleHook and the retarget/
  reconcile helper chain: they existed solely for supplement mode where
  hook-visible tools and the bridge coexisted. Exclusive mode keeps
  bridgeable tools out of the hook-visible record by design.
- Stale persisted payloads (feature_flags.json overrides, sendOptions/
  taskExperiments) are ignored, never rejected; covered by new
  stale-payload tests.
@ThomasK33

Copy link
Copy Markdown
Member Author

@codex review

@chatgpt-codex-connector

This comment has been minimized.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: cd9ffb14e9

ℹ️ 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".

Comment thread src/common/constants/experiments.ts
Comment thread src/node/services/toolAssembly.ts Outdated
Comment thread src/node/services/toolAssembly.ts
Comment thread src/node/services/toolAssembly.ts
Comment thread src/node/services/toolAssembly.ts
Comment thread src/node/services/toolAssembly.ts
Comment thread src/node/services/toolAssembly.ts
- Fail closed when PTC exclusive assembly fails (no silent flat fallback)
- Honor disable-all tool policies: skip code_execution synthesis when the
  policy leaves no tools (auto-compaction contract)
- Keep policy-required tools model-visible so stop-when conditions can
  observe their top-level toolResults
- Make memory/advisor (context-coupled) and attach_file/desktop_screenshot
  (media-producing) non-bridgeable so system-prompt context and media
  extraction keep working under the exclusive posture
- Elide base64 media payloads from bridged (MCP) content-container results
- Extract nested file_edit_* diffs/paths and agent_skill_read snapshots
  from code_execution records for compaction persistence
- Alias the legacy programmatic-tool-calling-exclusive override onto PTC on
  read and mirror it back on write (upgrade keeps the posture; downgrade
  runs exclusive instead of 2x supplement)
@ThomasK33

Copy link
Copy Markdown
Member Author

Addressed all 7 review findings in acac952:

  • Preserve the exclusive opt-in across version changes (experiments.ts): implemented a compatibility alias in ExperimentsService — a persisted programmatic-tool-calling-exclusive: true maps onto the merged PTC key at read time (upgrade keeps the posture), and an enabled PTC mirrors the legacy key back on write, so a downgraded build runs its exclusive posture instead of the removed ~2x supplement mode. Covered by new experimentsService.test.ts cases.
  • Fail closed when exclusive PTC assembly fails (toolAssembly.ts): the catch now throws for all PTC, not just RLM — no silent flat fallback.
  • Honor disable-all policies (toolAssembly.ts): code_execution is no longer synthesized when the policy leaves no tools (pinned by a new test using auto-compaction's .* disable rule).
  • Required bridge tools observable to stop conditions (toolAssembly.ts): policy-required tools are retained in the model-visible set (sourced from the grant+policy-filtered record), so createStopWhenCondition sees their top-level toolResults. New test.
  • Memory/advisor context (toolBridge.ts): memory and advisor are now non-bridgeable, so AIService's top-level availability checks (memory index/hot set, advisor guidance) keep working. New assembly test.
  • Media-producing tools (toolBridge.ts): attach_file and desktop_screenshot are now non-bridgeable (top-level media extraction keeps working), and bridged MCP content-container results get their base64 media items replaced with text placeholders before entering the sandbox/record. New bridge test.
  • Nested edit and skill state through compaction (extractEditedFiles.ts, loadedSkillSnapshots.ts): both extractors now consume successful nested PTC records, mirroring extractReadFiles. Classic PTC records recover full diffs/snapshots; kernel-compacted records surface edit paths (their result contents don't survive record compaction by design). New tests for both.

@ThomasK33

Copy link
Copy Markdown
Member Author

@codex review

@chatgpt-codex-connector

This comment has been minimized.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: acac9526a0

ℹ️ 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".

Comment thread src/common/schemas/project.ts Outdated
Comment thread src/node/services/experimentsService.ts Outdated
Comment thread src/node/services/ptc/toolBridge.ts Outdated
Comment thread src/common/utils/messages/extractEditedFiles.ts Outdated
…ases, nested media extraction, newest-first nested edit paths
@ThomasK33

Copy link
Copy Markdown
Member Author

@codex review

Round 2 findings addressed in 4d90398:

  • P1 taskExperiments: legacy programmaticToolCallingExclusive: true now aliases onto programmaticToolCalling when parsing persisted workspace configs, so resumed tasks keep exclusive PTC (and rlm stays effective).
  • P1 renderer localStorage: PTC toggles rewrite the legacy experiment:programmatic-tool-calling-exclusive key (both directions), and renderer reads alias a stored legacy true onto PTC — matching the backend alias semantics on upgrade and downgrade.
  • P2 bridged media: replaced bridge-side elision with request-time extraction — extractAttachmentsFromToolOutput now traverses code_execution toolCalls records, so nested bridged-MCP media becomes a model-visible attachment (placeholder in the record + synthetic user file part), identical to top-level tool media.
  • P2 nested edit ordering: extractEditedFilePaths traverses nested batches newest-first so large batches keep their latest edits under MAX_EDITED_FILES.

@chatgpt-codex-connector

This comment has been minimized.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 4d903985f2

ℹ️ 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".

Comment thread src/common/schemas/project.ts
Comment thread src/node/utils/messages/toolResultAttachments.ts Outdated
Comment thread src/browser/contexts/ExperimentsContext.tsx Outdated
Comment thread src/node/services/agentSkills/loadedSkillSnapshots.ts
Comment thread src/node/services/aiService.ts Outdated
…irror, kernel persistence records, outer-result media, bridge dedup
@ThomasK33

Copy link
Copy Markdown
Member Author

@codex review

Round 3 findings addressed in 62cd5fd:

  • P1 config loader: the legacy programmaticToolCallingExclusiveprogrammaticToolCalling alias now runs in normalizePersistedWorkspace (the actual loadConfigOrDefault path), retaining the legacy key for downgrade compatibility; the schema preprocess remains for parse paths.
  • P1 persisted-state helpers: the renderer legacy-key mirror now writes via updatePersistedState and reads via readPersistedState, joining the shared write-listener/subscriber notification path.
  • P1 RLM nested state: compactKernelToolCallRecords exempts agent_skill_read and file_edit_* records (bounded special records — result kept, args/error bounded), so loaded-skill snapshots and edited-file diffs survive compaction under PTC+RLM; legacy result-less records still degrade to path-only tracking.
  • P2 outer-result media: the code_execution extractor also rewrites the outer result and dedupes identical media between the record and return value into one synthetic attachment.
  • P2 bridge duplication: tools promoted to the model-visible set (policy-required, mcp_prompt_get) are now excluded from the ToolBridge, so every hook-visible tool has exactly one dispatch path and the no-reconcile premise holds.

@chatgpt-codex-connector

This comment has been minimized.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 62cd5fd830

ℹ️ 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".

Comment thread src/node/services/ptc/toolBridge.ts
Comment thread src/node/services/tools/code_execution.ts Outdated
Comment thread src/common/utils/messages/extractEditedFiles.ts
…ernel capture bounding, newest-first part traversal
@ThomasK33

Copy link
Copy Markdown
Member Author

@codex review

Round 4 findings addressed in 96503f6:

  • P1 capture bounding: KernelRecordBounds gained a resultExempt predicate consulted at record CREATION time, sharing one predicate (isKernelRecordResultExempt) with post-eval compaction — so agent_skill_read snapshots and file_edit_* diffs above the 16KB offload threshold (up to the ~50k-char tool caps) survive both stages. Args/errors stay bounded.
  • P2 RLM media: the same predicate exempts media-bearing content containers, giving bridged MCP media an RLM-safe path to request-time attachment extraction that doesn't depend on the static EXCLUDED_TOOLS list; the record carries the media even when the return value is offloaded to a vars handle.
  • P2 parts order: extractEditedFilePaths iterates message parts backward (mirroring extractReadFilePaths) before reversing each nested batch, so the newest execution's edits fill the cap first.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 96503f67f8

ℹ️ 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".

Comment thread src/node/services/ptc/types.ts Outdated
Comment thread src/node/config.ts
Comment thread src/common/utils/messages/extractEditedFiles.ts

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Security Review

Here are some automated security review suggestions for this pull request.

Reviewed commit: 96503f67f8

View security finding report

Only the user who started this review can view the report in Codex.

ℹ️ About Codex security reviews in GitHub

This is an experimental Codex feature. Security reviews are triggered when:

  • You comment "@codex security review"
  • A regular code review gets triggered (for example, "@codex review" or when a PR is opened), and you’re opted in so security review runs alongside code review

Once complete, Codex will leave suggestions, or a comment if no findings are found.

Comment thread src/node/services/ptc/types.ts
…safe nested edit records, extractable-only media exemption
@ThomasK33

Copy link
Copy Markdown
Member Author

@codex review

Round 5 findings addressed in 0918101:

  • P1 task snapshot mirror: toPersistedTaskExperiments stamps programmaticToolCallingExclusive: true alongside an enabled PTC at all task-spawn persistence sites (and the schema now declares the legacy field so parses retain it) — a downgraded build resumes such tasks in exclusive mode instead of supplement mode.
  • P1 nested result validation: collectNestedEditRecords skips records whose result is null or a primitive instead of throwing, keeping compaction preparation and attachment tracking self-healing against corrupt history rows.
  • P2 ×2 media exemption scope: containsMediaContentPayload now requires a supported attachment type (images/PDF/SVG), so audio/blob payloads stay under kernel bounding — and extractAttachmentsFromToolOutput replaces unsupported media parts with bounded placeholders at request time, so no raw base64 for unconsumable media reaches the provider from mixed containers or top-level results either.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 0918101860

ℹ️ 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".

Comment thread src/node/services/ptc/types.ts Outdated
Comment thread src/node/services/toolAssembly.ts
Comment thread src/cli/run.ts
Comment thread src/node/utils/messages/toolResultAttachments.ts Outdated
@ThomasK33

Copy link
Copy Markdown
Member Author

@codex review

Round 6 findings addressed in c862f0e + afa7f45:

  • P2 mixed media: the capture exemption became a retain-transform — KernelRecordBounds.captureRetained sanitizes mixed containers before retention, replacing unsupported parts (audio/blobs) with bounded placeholders while keeping extractable images/PDFs, so nothing unbounded persists to the record or chat.jsonl.
  • P2 allowlist: the no-tools check probes the policy with the synthesized code_execution name (applyToolPolicyToNames), so [disable .*, enable code_execution] keeps the exclusive entry point while a plain .* disable still yields no tools.
  • P2 CLI alias: both CLI collectExperiments implementations map programmatic-tool-calling-exclusive onto the merged PTC flag as a hidden compat alias, keeping upgraded automation working.
  • P2 console media: the code_execution extractor rewrites container-shaped consoleOutput args too, deduping the console.log(image) copy against record/return-value copies into one attachment.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: afa7f45d72

ℹ️ 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".

Comment thread src/common/orpc/schemas/stream.ts Outdated
Comment thread src/node/services/ptc/types.ts Outdated

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Security Review

Here are some automated security review suggestions for this pull request.

Reviewed commit: afa7f45d72

ℹ️ About Codex security reviews in GitHub

This is an experimental Codex feature. Security reviews are triggered when:

  • You comment "@codex security review"
  • A regular code review gets triggered (for example, "@codex review" or when a PR is opened), and you’re opted in so security review runs alongside code review

Once complete, Codex will leave suggestions, or a comment if no findings are found.

Comment thread src/node/services/ptc/types.ts
…ure-bounded persistence shapes, aggregate media budget

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Security Review

Here are some automated security review suggestions for this pull request.

Reviewed commit: 17c41950cc

ℹ️ About Codex security reviews in GitHub

This is an experimental Codex feature. Security reviews are triggered when:

  • You comment "@codex security review"
  • A regular code review gets triggered (for example, "@codex review" or when a PR is opened), and you’re opted in so security review runs alongside code review

Once complete, Codex will leave suggestions, or a comment if no findings are found.

Comment thread src/node/services/ptc/types.ts
…dAttachmentMediaType; bound metadata labels in provider placeholders
@ThomasK33

Copy link
Copy Markdown
Member Author

Round 11 finding addressed in fde549e:

  • Cap retained media types before request extraction: isSupportedAttachmentMediaType now requires a well-formed type/subtype (existing MEDIA_TYPE_PATTERN) within the existing 100-char length bound instead of a bare image/ prefix check — "image/" + ~3MiB fails validation at every consumer at once: capture retention (the part degrades to a bounded placeholder instead of being retained as supported media), request extraction, and provider output sanitization. As defense in depth, request-time placeholder builders additionally bound the interpolated media-type label (100 chars) and filename (200 chars) so even junk metadata in history persisted by earlier builds cannot bloat later provider requests, and extracted-attachment filenames are bounded too.

Validation: new unit tests — junk-MIME parts rejected at validation with bounded placeholder labels; serialized-charge backstop re-verified through well-formed parts hiding megabytes in filename (including the multibyte UTF-8 case); extraction-level test proving junk media-type/filename metadata collapses to bounded placeholders (<2KB output, no synthetic attachment). make static-check, PTC suites, and validator-consumer suites green.

@ThomasK33

Copy link
Copy Markdown
Member Author

@codex review

@chatgpt-codex-connector

This comment has been minimized.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: fde549ec3f

ℹ️ 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".

Comment thread src/node/utils/messages/toolResultAttachments.ts Outdated
Comment thread src/node/services/tools/code_execution.ts Outdated
…e-bounded markers normally (preserve path attribution + failed-edit success bit)
@ThomasK33

Copy link
Copy Markdown
Member Author

Round 12 findings addressed in 60f9bbf:

  • P1 — Bound recursion through nested tool-call results: the mutual recursion between extractAttachmentsFromToolOutput and extractAttachmentsFromNestedToolCalls (json wrappers included) now carries a depth counter capped at 64; over-deep values are left unrewritten instead of recursed into, so a syntactically valid but corrupt chat.jsonl row with a deep {toolCalls:[{result: …}]} chain degrades to no extraction rather than stack-overflowing every subsequent provider request (self-healing rule). Covered by a test feeding a 50,000-deep chain (genuinely overflows without the fix).
  • P2 — Preserve edit attribution after retained-budget overflow: compactKernelToolCallRecords now detects __kernelBounded markers before the name-based persistence exemption and compacts them normally — the result-less {ok, bytes} summary keeps crash-safe edited-file tracking (path from args) and reports the honest size instead of preserving a marker that extractors reject. Additionally, the capture-time budget fallback now preserves a boolean success bit onto the marker, so a FAILED oversized edit after overflow compacts to ok:false instead of misreporting ok:true and advertising a never-applied path.

Validation: extended the execution-budget integration test with post-overflow successful and failed edits (asserting {ok, bytes} compaction, path attribution, and the failure bit); new deep-recursion extraction test; make static-check plus PTC/messages/extraction suites green.

@ThomasK33

Copy link
Copy Markdown
Member Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 60f9bbf4a9

ℹ️ 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".

Comment thread src/node/utils/messages/toolResultAttachments.ts Outdated
@ThomasK33

Copy link
Copy Markdown
Member Author

Round 13 finding addressed in b1c0ae2:

  • Replace over-depth history nodes instead of retaining them: the depth guard now returns a bounded placeholder rewrite ([tool output omitted from provider request: nested tool-record depth limit exceeded]) instead of null, so callers replace the over-deep subtree in the provider copy rather than retaining it — a payload hiding at the leaf can no longer ship as raw JSON on every later request. Replacement is precisely targeted because descent only follows tool-output-shaped wrappers ({type:"json"} and toolCalls chains): anything past the cap is malformed by construction. Persisted history itself is never mutated.

Validation: updated the deep-recursion test to bury a media payload at the leaf of a 50,000-deep chain and assert the provider copy contains the placeholder and not the payload; make static-check, extraction/messages suites, code_execution, and post-compaction attachment suites green.

@ThomasK33

Copy link
Copy Markdown
Member Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: b1c0ae24b0

ℹ️ 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".

Comment thread src/common/utils/messages/extractEditedFiles.ts Outdated
@ThomasK33

Copy link
Copy Markdown
Member Author

Round 14 finding addressed in 8925af2:

  • P1 — Validate nested diff values before parsing: collectNestedEditRecords now admits a nested diff only when typeof diff === "string" — a malformed persisted row carrying an array/object diff degrades to a path-only edit record (the successful edit still counts for crash-safe path tracking) instead of reaching parsePatch/applyPatch and throwing on every compaction/recovery pass. The direct (non-nested) file_edit_* part path in extractEditedFileDiffs had the same latent truthiness hole (if (!diff) passes a non-empty array), so it received the identical string-only guard.

Validation: new test covering both paths (nested array diff + direct object diff): paths still tracked, corrupt diffs dropped, no throw, and the well-formed sibling diff still combines. make static-check and messages/post-compaction suites green.

@ThomasK33

Copy link
Copy Markdown
Member Author

@codex review

@chatgpt-codex-connector

This comment has been minimized.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 8925af27a3

ℹ️ 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".

Comment thread src/node/utils/messages/toolResultAttachments.ts
@ThomasK33

Copy link
Copy Markdown
Member Author

Round 15 finding addressed in ef0e7e0:

  • P1 — Traverse media inside wrapped outer results: request-time extraction now deep-walks arbitrary wrapper objects/arrays (extractAttachmentsFromWrapperValue, routed through the extractAttachmentsFromToolOutput fallback so the shared 64-level depth cap bounds the stack and over-deep subtrees still degrade to the bounded placeholder). return { image: xum.mcp(...) } — and the console-arg equivalent console.log({ wrapped }) — now rewrite the retained supported container into the attachment placeholder in the provider copy wherever it sits, deduplicating against the nested-record copy, instead of shipping the screenshot as both an attachment and megabytes of raw JSON on every later request. Sibling wrapper fields pass through untouched, and persisted history is never mutated.

Validation: new test with the container wrapped in the outer result AND a console arg alongside the nested record — all three copies rewritten, one deduped attachment, sibling fields intact; deep-recursion and prior extraction tests unaffected; make static-check, messages/extraction suites (247 tests), code_execution, and post-compaction suites green.

@ThomasK33

Copy link
Copy Markdown
Member Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: ef0e7e07e2

ℹ️ 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".

Comment thread src/node/utils/messages/toolResultAttachments.ts
Comment thread src/common/utils/messages/extractEditedFiles.ts

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Security Review

Here are some automated security review suggestions for this pull request.

Reviewed commit: ef0e7e07e2

ℹ️ About Codex security reviews in GitHub

This is an experimental Codex feature. Security reviews are triggered when:

  • You comment "@codex security review"
  • A regular code review gets triggered (for example, "@codex review" or when a PR is opened), and you’re opted in so security review runs alongside code review

Once complete, Codex will leave suggestions, or a comment if no findings are found.

Comment thread src/node/utils/messages/toolResultAttachments.ts
…e positive success bit for result-less edit/read records
@ThomasK33

Copy link
Copy Markdown
Member Author

Round 16 findings addressed in edef2b8:

  • Avoid truncating deep outputs that contain no media: the generic wrapper walk now stops at the depth cap and leaves the subtree unchanged (extractAttachmentsFromWrapperValue returns null at the cap, and its children re-enter at depth + 1 ≤ cap), so the over-depth placeholder is reachable only through tool-output-shaped edges (json wrappers, toolCalls record chains) that add further depth — exactly the shapes that are malformed by construction past the cap. Media-free deep JSON passes through untouched (new test: 200-deep object output, reference-equal after extraction), while the corrupt-chain placeholder tests still hold.
  • Require a positive compact success bit: collectNestedEditRecords now skips result-less records unless ok === true — valid kernel-compacted records always carry an explicit boolean ok and classic records carry a result, so {toolName:"file_edit_insert", args:{path}} no longer reports a completed edit. The sibling read-tracking extractor (collectNestedReadPaths) had the identical inference-from-absence hole (a malformed row would advertise a never-read path), so it received the same guard; the read-test load fixture was updated to model real loadActive compaction output (result kept, no ok bit).

Validation: new tests for the malformed edit record, the malformed read record, and untouched deep JSON; make static-check, messages/extraction suites (249 tests), code_execution, and post-compaction suites green.

@ThomasK33

Copy link
Copy Markdown
Member Author

@codex review

@ThomasK33

Copy link
Copy Markdown
Member Author

Round 16 security straggler addressed in 5472882:

  • Continue walking wrapper siblings after nested extraction: extractAttachmentsFromNestedToolCalls now merges a sibling walk into the nested rewrite — every own field beyond toolCalls/result/consoleOutput is routed through extractAttachmentsFromToolOutput (shared depth cap, deduped via the same pushUnique), so a wrapper holding BOTH a media-bearing toolCalls array and another media-bearing sibling field has all copies rewritten instead of the early return skipping siblings.

Validation: the wrapped-media test now includes a sibling field beside the toolCalls structure (all four copies rewritten, one deduped attachment, non-media fields intact); make static-check, messages/extraction suites (249 tests), code_execution, and post-compaction suites green.

@ThomasK33

Copy link
Copy Markdown
Member Author

@codex review

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