Environment and compaction hygiene from the #48 and #57 reviews (invariants, guards, consistency, tests, docs) #54

Finding

Found in the #48 review. The code is from upstream miuuyy#884 plus the fork's #28 resolution in #48.

In src/adapters/chatgpt-web/thread-environment.ts (resolve recovery path), the calendar delta now reaches the root logic through two channels:

const calendarDelta = hasCurrentContext && !currentCompaction
  ? extractChatGptCalendarEnvironmentDelta(parsed) : undefined;
...
const resolvedCalendarDelta = calendarDelta ?? currentClaim?.calendarDelta;

• The local calendarDelta is gated by !currentCompaction.
• currentClaim?.calendarDelta is not gated. Today only extractChatGptContinuationEnvironmentClaim sets it, and that runs only when currentCompaction is true.

That makes the code safe today: conflictingAuthorityFields(currentClaim, rolloutEnvironment) bounds the resulting roots to the claim's own roots, and #48's tests pin that. But the invariant is implicit. A future change that makes another claim source carry calendarDelta would apply the #28 context-root adoption, contextRoots, without the !currentCompaction gate, and no test would notice.

Expected

Make the invariant explicit, for example:

• type calendarDelta as present only on the continuation claim (a separate return type), or
• derive resolvedCalendarDelta explicitly (currentCompaction ? currentClaim?.calendarDelta : calendarDelta), with a test that a steering or native-compaction claim carrying calendarDelta is ignored or rejected.

Coordinate with upstream: the two-channel shape comes from miuuyy#884.

Only the Guard limits and Not an issue sections of #58 belong here. Its Behavior section moved to #52.

Follow-up items from the final review of #57 (#29). The reviewer left them out of scope, and they are not recorded in #29 or #50.

Behavior

• Root order in a midnight delta versus the broker. This predates fix(environment): keep established roots and spelling across midnight deltas (#29) #57.
  • calendarContextRoots (src/adapters/chatgpt-web/thread-environment.ts, the latest branch) compares the delta roots with the native roots as sets, then returns the delta's list in the delta's order.
  • Scenario: a full-access rollout that omits workspace_roots, a turn that started with roots [cwd, sibling], and a midnight delta that repeats them as [sibling, cwd].
  • The resolved environment then differs from the pinned turn-start environment only in order. TurnBroker.updateEnvironment compares environmentIdentity with JSON, so it aborts with "environment changed during an active ChatGPT tool loop".
  • This is the same family as the spelling issue that fix(environment): keep established roots and spelling across midnight deltas (#29) #57 fixed with preferSpelling.
  • A possible fix: when the cached entry has the same set of identities, return it in the cached order and spelling.

Guard limits

• ..:x now counts as inside its root. fix(environment): keep established roots and spelling across midnight deltas (#29) #57 changed the three lexical checks (contains in thread-environment.ts and codex-rollout-environment.ts, matchesPath in environment.ts). Only .. itself or a leading .. path segment is now outside.
  • relative(root, root + "\..:x") is "..:x". That was outside before fix(environment): keep established roots and spelling across midnight deltas (#29) #57 and is inside now.
  • The risk is low: Windows cannot name a directory entry .., and these checks only bound workspace context, not sandbox grants.
  • Confirm that this is acceptable, or reject segments that start with ..:.
• C0 controls in bullet worker names. fix(environment): keep established roots and spelling across midnight deltas (#29) #57 rejects NEL and bidi controls, but the name class still accepts other C0 controls such as ESC (U+001B) and NUL (U+0000). The risk is the same as before: names are discarded and grant nothing. Allowing only printable characters would cover these too.

Not an issue (recorded for completeness)

• ... and .. (trailing dots or spaces) count as inside the root. Win32 strips trailing dots and spaces from the last segment, so these names resolve to the root itself.

Findings from the second full review of #57 that are not tracked elsewhere. The review's other findings are either fixed in #57 (8b67b20, the broker's ..notes cwd check) or already tracked: the <agent name> form in #29, and the duplicated containment helpers in #50.

Consistency

• The visualization-root check still uses the old .. prefix test.
  • Location: isCurrentOrParentThreadVisualizationRoot in src/adapters/chatgpt-web/environment.ts (if (!rel || rel.startsWith("..") || isAbsolute(rel)) return false;).
  • Every other containment check in fix(environment): keep established roots and spelling across midnight deltas (#29) #57 now treats only .. itself or a leading .. path segment as outside.
  • No behavior impact today: the accepted shape is <codexHome>/visualizations/YYYY/MM/DD/<thread id>, so a first segment starting with .. already fails the 4-digit year check.
  • Aligning it would remove the last copy of the old predicate. Ideally this happens together with the shared helper proposed in Propose Electron 43, the launcher postinstall and the Windows root comparison fix upstream #50.

Design decision to record

• Cached spelling wins over a full environment claim in the same request.
  • Location: thread-environment.ts, the preferSpelling block after calendarContextRoots.
  • On a calendar-delta turn, a cached entry's spelling replaces the spelling of paths with the same identity, even when the request also carries a full environment (compaction continuation or steering).
  • Current rationale: the broker compares later environments with the pinned turn-start environment, which is the cached one, by exact JSON. Preferring the cache keeps the tool loop alive. Identities, and therefore authority, are unchanged.
  • Confirm: a compaction continuation that starts a new broker registration would pin the request's spelling instead, so it should not get the cached one. Check whether any path re-registers with this resolved environment and could expect the request's own spelling.

Minor efficiency and readability

• peek() runs twice on a rootless calendar-delta turn. It runs once inside calendarContextRoots and once in the spelling block, with the same lookup and TTL check.
  • The cost is negligible.
  • Reading the entry once and passing it to calendarContextRoots would make the cache dependence of both steps explicit.

Finding

The #48 reviews left minor test-coverage gaps in the compaction plus midnight tests in tests/environment.test.ts (added in 2e3fc06 and c25bf4b). None blocks the merge: the security properties are pinned, as mutation checks confirmed.

Tests

• Test B covers only a trusted Codex home. The refusesContext guard on the compaction-carried delta path is pinned only for codexHome. User home, filesystem/drive root, UNC/namespaced spellings and the bridge data directory are covered by fix(environment): accept Codex midnight world-state deltas and guard broad context roots #28 tests only outside the compaction path. Consider parametrizing B over those roots.
• Test C asserts only .cwd. It mirrors the existing stream disconnected before completion #27 bullet-inventory test. Asserting the whole resolved environment (toEqual) would also pin roots and the sandbox policy.
• Test A does not separate the two resolutions on POSIX.
  • On Linux/macOS it would also pass with upstream's strict exact-roots block.
  • On Windows it fails with that block only because of the case bug described in Propose Electron 43, the launcher postinstall and the Windows root comparison fix upstream #50, not because of the fix(environment): accept Codex midnight world-state deltas and guard broad context roots #28 relaxation.
  • That is acceptable, because conflictingAuthorityFields bounds the compaction path to the claim's roots and B/D pin the guards. A Windows-specific test that pins the case-insensitive root comparison would make the difference explicit.

Docs

docs/superpowers/plans/2026-10-22-issue46-upstream-sync.md has rationale the execution corrected; it should be annotated or amended:

• Task 2 step 1 asks for delta roots "that the preamble does not list". That is impossible by design (conflictingAuthorityFields).
• Task 2 step 2 expects test A to fail under upstream's strict block because of the relaxation. It fails only on Windows, because of the case bug.
• Task 3 step 1 assumes bun run verify is green. It is red because of the root bun audit advisory.

Finding

Found in the #48 review, in code brought in from upstream (4ad190f "Preserve agent-only child compaction without a nonexistent human alias", 59b5b0f).

In src/server.ts (~line 719-729), the comment above the compaction checkpoint still says:

v1 installs the bounded user-message output, whereas v2 retains the original source. Authenticate both exact producer-defined representations, never arbitrary rewrites.

The code now calls findChatGptCompactionSourceRevision for the v1 representation. When that returns undefined (a child with only parent instructions and contextual items, no human source), rememberCompactionContinuation receives only [source], so only one representation is authenticated in that case.

The behavior looks intentional: upstream added it so agent-only child compactions do not need a nonexistent human alias. The comment no longer describes it, though, and nothing states that skipping the v1 alias is the intended, safe outcome.

Expected

• Update the comment to state when the v1 alias is omitted and why that is safe.
• Optionally, add an assertion or test that the single-alias checkpoint is only produced for the agent-only child case.
• If upstream changes this comment, take their wording on the next sync instead, to avoid a conflict.