Skip to content

Fix console workspace changes across agents - #1779

Merged
gjkim42 merged 1 commit into
mainfrom
fix/console-workspace-changes
Sep 29, 2026
Merged

gjkim42 merged 1 commit into
mainfrom
fix/console-workspace-changes

Conversation

@gjkim42

@gjkim42 gjkim42 commented Sep 29, 2026 •

Copy link
Copy Markdown
Collaborator

What type of PR is this?

/kind bug

What this PR does / why we need it:

The console Changes tab depends on provider diffs, so newly created files can be missing for Claude Code and OpenCode, reverted files can remain visible, and loading conversation history can change the list.

Use a shared runtime workspace snapshot for every provider. Compare against a persisted Git base, include committed, staged, unstaged, and non-ignored untracked files, and leave the user's staging area unchanged. Apply workspace snapshots independently of conversation diffs, preserving the displayed diff when a snapshot is unchanged. Refresh after connecting and completing turns, when opening Changes, and every five seconds while it is visible. Limit snapshots to 1,000 files, a bounded encoded file list, and 1 MiB of diff content so they stay below the console transport limit. Report truncation and Git diagnostics explicitly, and retain readable files when a nested repository has no commits. Git inspection failures do not prevent Sessions from starting.

The screenshots use the same browser fixture: an edited README and a newly created setup document.

Before — the new setup document is missing.

Before: Changes lists only the modified README

After — both files appear, with the comparison base displayed.

After: Changes includes the modified README and new setup document

Which issue(s) this PR is related to:

N/A

Special notes for your reviewer:

  • Git compatibility: unborn nested repositories are excluded before intent-to-add. Tests cover both Git 2.43.0 and Git 2.55.0, preserve valid neighboring repositories when names contain glob characters, and confirm the real index is unchanged. The full unit suite passes with the race detector, and the focused workspace tests also pass with the race detector on Git 2.55.0.
  • The runtime attempts to record the base before the provider starts and retries when a workspace snapshot is requested if Git inspection is unavailable. The saved base survives runtime restarts. Existing Sessions establish their base on the first successful inspection after upgrading; existing uncommitted changes are included. Renames are shown as a deletion and an addition.
  • Tests cover all three providers, staging-area preservation, commits and restarts, reverts, empty and binary files, file/symlink conversions, unusual filenames, unborn and nested repositories, encoded payload bounds, Git initialization errors, reconnects, unchanged DOM preservation, and transcript paging. The unused projected-history fileDiff field and its computation are removed.
  • Validation: make update, env -u CODEX_HOME -u CODEX_AUTH_JSON make test, make verify, and make build passed. The environment exclusions isolate existing entrypoint tests from the running Codex session.
  • Browser validation used the actual console assets with mocked API and runtime responses. Screenshots are PR attachments, not repository files. Cluster e2e tests are left to PR CI.

Does this PR introduce a user-facing change?

The console Changes tab now shows a consistent workspace comparison for Codex, Claude Code, and OpenCode, including newly created, staged, and committed files. It refreshes terminal edits while visible, removes reverted changes, and remains independent of conversation history.

@gjkim42 gjkim42 added the kind/bug Categorizes issue or PR as related to a bug label Sep 29, 2026
@gjkim42

gjkim42 commented Sep 29, 2026

Copy link
Copy Markdown
Collaborator Author

/kelos review

@kelos-bot

kelos-bot Bot commented Sep 29, 2026 •

Copy link
Copy Markdown
Contributor

🤖 Kelos Task Status

Task kelos-reviewer-issue-comment-55492ed0b4ae has succeeded. ✅

@kelos-bot

kelos-bot Bot commented Sep 29, 2026

Copy link
Copy Markdown
Contributor

🤖 Kelos Reviewer Agent @gjkim42

Review Summary

Verdict: COMMENT (sticky comment only)
Overall correctness: patch is correct
Scope: Replaces the provider-diff–driven Changes tab with a runtime workspace.changes snapshot, compared against a Git base stored in the Session state dir, for all agent types.

Findings Overview

Priority Count File:Line Summary
P0 0 — none
P1 0 — none
P2 2 internal/sessionruntime/workspace_changes.go:154 Snapshot size is unbounded; a large change set exceeds the console's 8 MiB line limit and causes repeated reconnects
P2 2 internal/sessionruntime/workspace_changes.go:143 A nested repo with no commits makes git add -N . fail on every capture
P3 3 internal/sessionruntime/workspace_changes.go:144 Git errors show only exit status N because stderr is dropped
P3 3 internal/sessionruntime/workspace_changes.go:183 File↔symlink type changes lose the deletion half of the diff
P3 3 internal/sessionruntime/protocol.go:95 HistoryState.FileDiff is still computed and sent, but nothing reads it

Findings

Correctness

  • [P2] internal/sessionruntime/workspace_changes.go:154 — The 1 MiB cap applies only to diff text. Every changed path is still sent, each with a 66-byte placeholder, in a single JSON line. The console bridge reads runtime output with an 8 MiB per-line limit (internal/consoleserver/server.go:1758). The escaped diff also counts toward that limit (<, >, and & become 6 bytes each). With roughly 50k or more changed or non-ignored untracked files, such as a dependency or build directory that .gitignore misses, the scanner fails with token too long and bridgeExec closes the socket. history.end requests a snapshot on every connect (internal/consoleserver/frontend/app.ts:4167), so the client reconnects, fails again, and loops. The conversation becomes unusable even if the Changes tab is never opened. Suggest capping the listed files (or the total encoded size) well below the bridge limit, reporting the cut through Truncated/Message, and adding a test with more files than the cap.
  • [P2] internal/sessionruntime/workspace_changes.go:143 — git add --intent-to-add -- . exits 128 with '<dir>/' does not have a commit checked out when the workspace contains a nested repository with no commits, e.g. a subdirectory where the agent ran git init. Reproduced with git 2.43. Every capture then fails, and the tab shows only an error with no file list until that directory gets a commit or is removed. --ignore-errors makes git add the remaining paths, but it still exits 1, so the call also has to accept that status. Alternatively, list untracked files separately. Please add a test with an unborn nested repository.

Error reporting

  • [P3] internal/sessionruntime/workspace_changes.go:144 — *exec.ExitError does not include stderr in Error(), so .Run() and .Output() failures reach the UI as Unable to read workspace changes: collecting workspace files: exit status 128. The PR says collection errors are displayed explicitly, but users can't see the cause (e.g. a missing base commit, the nested-repo case above, or a lock). Include trimmed stderr in the wrapped errors, via CombinedOutput or ExitError.Stderr.

Diff mapping

  • [P3] internal/sessionruntime/workspace_changes.go:183 — For a file↔symlink type change, git emits two diff --git segments for the same path: a deletion, then a creation (verified). patches[path] = content overwrites the first, so the entry shows only the new symlink and loses the removed content and its − count. Append segments for the same path instead of overwriting.

Cleanup

  • [P3] internal/sessionruntime/protocol.go:95, internal/sessionruntime/history.go:66-118 — HistoryState.FileDiff only fed the Changes tab, and the console no longer reads it (fileDiff was removed from the TS HistoryState). The runtime still computes and sends up to 32 KiB of it on every initial history load, and internal/sessionruntime/server_test.go:2852 still asserts on it. Remove the field, its computation in history.go, and the assertion, so the protocol no longer carries data from the old approach.

Key takeaways

  • The design is sound. The temporary index keeps the user's staging area intact, merge conflicts diff correctly (verified), and tests cover the happy path across all three providers.
  • The main risk is payload size. Keep every workspace.changes event safely under the console bridge's 8 MiB line limit, so a large workspace cannot break the whole Session connection.

@cubic-dev-ai cubic-dev-ai 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.

All reported issues were addressed across 11 files

Reply with feedback, questions, or to request a fix.

Re-trigger cubic

Comment thread internal/consoleserver/web/app.js Resolved
Comment thread internal/sessionruntime/server.go Resolved
Comment thread internal/sessionruntime/server.go Resolved
Comment thread internal/consoleserver/frontend/app.ts Resolved
@gjkim42

gjkim42 commented Sep 29, 2026

Copy link
Copy Markdown
Collaborator Author

Addressed the five Kelos findings and two applicable Cubic findings:

  • Bound the file count and encoded file-list size alongside the diff limit; tests exercise JSON escaping and keep the event below the console bridge limit.
  • Preserve readable changes when an unborn nested repository cannot be added, with a visible diagnostic.
  • Include bounded Git stderr in collection errors and keep Git inspection failures from aborting Session startup.
  • Preserve both patch segments for file/symlink conversions.
  • Remove the unused projected-history fileDiff field and computation.
  • Preserve the existing diff DOM for unchanged snapshots while still servicing pending refreshes.

The two other Cubic findings were checked against the current code. Console requests already coalesce to one outstanding capture and reject stale request IDs. Transcript resets retain a valid workspace request; connection shutdown clears it through reconnect handling. Details are in the corresponding review threads.

Validation passed: make update, env -u CODEX_HOME -u CODEX_AUTH_JSON make test, make verify, and make build. The environment exclusions isolate existing entrypoint tests from the running Codex session. The branch has been squashed to one commit, and the PR description has been updated. Fresh CI will validate the pushed head.

@cubic-dev-ai cubic-dev-ai 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.

1 issue found across 9 files (changes from recent commits).

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="internal/sessionruntime/workspace_changes.go">

<violation number="1" location="internal/sessionruntime/workspace_changes.go:187">
P2: The file-count and encoded-list limits are applied only after Git’s complete name list is buffered and split, so a workspace with millions of changes can exhaust runtime memory despite the snapshot caps. Stream or cap the listing before materializing it, while retaining enough information for the truncation diagnostic.</violation>
</file>

Reply with feedback, questions, or to request a fix.

Re-trigger cubic

changes.Truncated = true
}
args := []string{"diff", "--no-ext-diff", "--no-textconv", "--no-color", "--no-renames", "--src-prefix=a/", "--dst-prefix=b/", base}
output, err := c.output(ctx, root, environment, append(args, "--name-only", "-z", "--")...)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2: The file-count and encoded-list limits are applied only after Git’s complete name list is buffered and split, so a workspace with millions of changes can exhaust runtime memory despite the snapshot caps. Stream or cap the listing before materializing it, while retaining enough information for the truncation diagnostic.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At internal/sessionruntime/workspace_changes.go, line 187:

<comment>The file-count and encoded-list limits are applied only after Git’s complete name list is buffered and split, so a workspace with millions of changes can exhaust runtime memory despite the snapshot caps. Stream or cap the listing before materializing it, while retaining enough information for the truncation diagnostic.</comment>

<file context>
@@ -133,34 +168,55 @@ func (c *workspaceChangesCollector) capture(ctx context.Context) (WorkspaceChang
 	}
 	args := []string{"diff", "--no-ext-diff", "--no-textconv", "--no-color", "--no-renames", "--src-prefix=a/", "--dst-prefix=b/", base}
-	output, err := c.command(ctx, root, environment, append(args, "--name-only", "-z", "--")...).Output()
+	output, err := c.output(ctx, root, environment, append(args, "--name-only", "-z", "--")...)
 	if err != nil {
 		return changes, fmt.Errorf("listing workspace changes: %w", err)
</file context>

@gjkim42
gjkim42 force-pushed the fix/console-workspace-changes branch from b470b00 to 3091605 Compare September 29, 2026 15:44
@gjkim42

gjkim42 commented Sep 29, 2026

Copy link
Copy Markdown
Collaborator Author

Fixed the failing TestWorkspaceChangesIncludesOtherFilesWithUnbornNestedRepository test in 3091605.

Git 2.55.0 accepts an unborn nested repository into the intent-to-add index, then fails during patch generation with fatal: cannot hash nested. The collector now excludes unreadable nested repositories before intent-to-add and reports the omission. The expanded test verifies that valid neighboring repositories remain visible, bracketed names are handled literally, and the real index is unchanged.

Validation passed locally: the full race-enabled unit suite, focused workspace tests with the race detector on Git 2.55.0, make verify, and make build. The fresh CI unit-test job also passed, along with CI build and verification. Integration and e2e are still running.

The PR remains squashed to one commit.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant