Skip to content

Preserve assistant newline semantics after #584#587

Draft
baron wants to merge 2 commits into
mainfrom
feature/restore-assistant-newline-semantics
Draft

Preserve assistant newline semantics after #584#587
baron wants to merge 2 commits into
mainfrom
feature/restore-assistant-newline-semantics

Conversation

@baron

@baron baron commented Jul 19, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • replace PR #584's Character.isNewline loop with a hoisted CharacterSet.newlines scalar loop
  • preserve the established Foundation newline-scalar contract, including counting CRLF as two separators
  • add a dedicated five-test, 26-scenario contract matrix for newline membership, edge cases, exactness, and preview agreement

Why this follows #584

This is a corrective follow-up to PR #584, not a rejection of its performance finding or a git revert.

#584 correctly identified a hot loop, but changed its iteration unit from Unicode scalars to extended-grapheme Character values. Swift exposes CRLF as one newline Character, while the unchanged previewSummary still uses components(separatedBy: .newlines), where CR and LF are separate Foundation newline separators.

At the 10-line collapse boundary, merged #584 can therefore report:

  • bounded decision: 10 lines, exact
  • preview summary: 11 lines, collapse required

That one-sided semantic change can suppress the collapsed assistant-message presentation.

The smallest correction is:

let newlines = CharacterSet.newlines
for scalar in text.unicodeScalars where newlines.contains(scalar) {

This preserves the pre-#584 scalar/CRLF behavior and realigns the decision with the existing preview contract.

Confirmed performance mechanism

Release-optimized SIL shows why the original inline form was slow: the Foundation CharacterSet.newlines getter, temporary allocation, destruction, and deallocation remain inside the scalar loop. The compiler does not hoist them across the Foundation boundary. Binding the set once removes that repeated work and avoids extended-grapheme segmentation.

The expert's 1M ASCII result was approximately:

Original inline set #584 Character.isNewline Hoisted scalar set
335 ms 40 ms 30 ms

Two independent optimized reproductions confirmed the ordering:

Run Original inline set #584 Character.isNewline Hoisted scalar set
1 200.7 ms 30.5 ms 21.5 ms
2 274.9 ms 44.1 ms 26.7 ms

The hoisted scalar form also won on combining and emoji/ZWJ no-newline inputs. Character.isNewline retained a sub-microsecond advantage on one LF-dense early-exit shape, so this PR does not claim universal dominance; it fixes the reported long-line case while preserving the established contract.

Focused coverage

AgentAssistantLineDerivationTests protects 26 scenarios across five methods:

  • LF, CR, VT, FF, NEL, U+2028, and U+2029
  • CRLF explicitly counted as two scalar separators, including the 10-line collapse boundary
  • empty input and every trailing-separator form
  • combining and emoji/ZWJ graphemes without newlines
  • limits 0, 9, 10, and 11 with exact count / isExact expectations
  • bounded collapse-decision agreement with previewSummary

The CRLF boundary test was demonstrated red on merged #584 (10/exact versus 11/collapse) before the production correction.

Validation

Passed:

  • make dev-test FILTER=RepoPromptTests.AgentAssistantLineDerivationTests: 5 tests, 0 failures
  • authoritative root XCTest list contains all five exact IDs
  • ledger verification: 3,442 executable entries reconciled
  • make dev-lint
  • make dev-swift-build PRODUCT=RepoPrompt
  • commit/push contribution preflights, guardrails, and staged/outgoing secret scans

Known limitations (hence draft):

  • a prior 55-minute full-root run on the branch's Fix agent message bubble line counting performance #584-era base completed with 24 failures in CodeMap/worktree timing, authority, and tool-catalog golden tests outside the changed surface; the tool-catalog golden was subsequently updated on main by Fix history tool catalog golden #585
  • isolated reruns reproduced one CodeMap authority failure on that base
  • non-disruptive live smoke could not connect because the running CE app did not expose MCP; no visible app launch or relaunch was performed

Intentionally separate follow-ups

This PR does not broaden the corrective boundary:

  1. Replace previewSummary's full components array with a reviewed scalar-contract single-pass derivation; exact remaining-line counts still require O(n) scanning even if retained allocation is bounded.
  2. Make GitService.isHexObjectID ASCII-hex strict; Character.isNumber currently accepts Unicode digits.
  3. Profile large-streaming Markdown rescans before redesigning them.
  4. Do not introduce a generic repository-wide line abstraction; existing byte/scalar/Character/display contracts intentionally differ.

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.

2 participants