sync: port upstream server settle, tunnel/update safety, and early web polish (#5482–#5486) - #215
Conversation
…ingdotgg#5482) Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
…pingdotgg#5484) Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
The version-skew banner is no longer an amber warning: it reads "Server update available" with the raw versions (unreadable for nightlies) moved to a tooltip. The in-flight rail (Download/Install/ Resume) becomes a single status row, "Downloading…" then "Restarting…", since the wire installing stage is a sub-second launcher handoff and "resuming" meant nothing to most people. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Measured on a live update: the server was back in ~9s but stayed unreachable for ~96s, because releasing the tunnel on shutdown forces the replacement tunnel's hostname route through 1-2 minutes of edge propagation. An update handoff always brings a server right back (new version or rollback), so the tunnel is never orphaned; skip the release when the launcher state file records a pending update. The next boot respawns the connector from the stored config against the same tunnel and is reachable as soon as it connects. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…pdate A pending update in the launcher state is not proof a replacement server is coming: `t3 service uninstall` or `systemctl stop` during the pending window also tears the server down, permanently. The launcher now writes a stop marker before signalling its child on an explicit stop and clears it on the next start; the shutdown tunnel release keeps the tunnel only for pending updates without the marker. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
A tool.updated row is the in-flight snapshot of a call; once the call completes, the tool.completed row carries the final state and both clients fold every matching update into it. Shipping the updates buys nothing: 47k such rows exist in one real database, and a single thread carries 3,291 of them. Filter them out of thread snapshots, mirroring the existing context-window dedup. Matching is per turn and only against a LATER completion, so a revert that discards the completing turn cannot leave a call unrepresented, and a later update under the same identity (the next call, still in flight) survives. Live events are untouched. Rows are matched on the same identity the clients collapse by: an explicit data.toolCallId when the adapter emits one, otherwise the itemType/title/detail triple. No tool lifecycle row in the real db carries a toolCallId, so the fallback does the work. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… test Bugbot flagged that clients collapse only adjacent lifecycle rows, so dropping a superseded update separated by an interleaved parallel call diverges from full-history rendering. Measured on a real database: 1.5% of dropped rows (553/36,581), all pure in-flight state whose final result the retained completion still shows, and zero dropped rows carry a client-merged payload field their completion lacks (verified across all 49,515 update rows). Documents the tradeoff and adds a test pinning it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-authored-by: t3-code[bot] <269035359+t3-code[bot]@users.noreply.github.com>
Co-authored-by: t3-code[bot] <269035359+t3-code[bot]@users.noreply.github.com>
…dotgg#5450) Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Co-authored-by: aaditagrawal <aaditagrawal@users.noreply.github.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughChangesCloud service handoff
Activity payload projection
ACP approval classification
Server update interface
Snooze and wake interaction
Interface presentation polish
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
Bot
added
vouch:trusted
Co-authored-by: aaditagrawal <aaditagrawal@users.noreply.github.com>
|
@coderabbitai review |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (3)
apps/server/src/orchestration/ActivityPayloadProjection.test.ts (1)
79-99: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the MCP
filesderivation.
projectMcpToolCallDatanow emits afilesarray for MCP payloads. Previously MCP data passed through unchanged, sofilesis a new key on the wire for this item type. Neither MCP test here nor the fixture inapps/server/test/ActivityPayloadProjection.test.tscontains a path-like key, so the branch never runs for MCP.Add a case with a path in
inputoritem.argumentsand assert the projectedfilesentries.Guidelines require focused tests for changed backend behavior.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/server/src/orchestration/ActivityPayloadProjection.test.ts` around lines 79 - 99, Extend the MCP projection test around projectActivityPayload to include a path-like value in input or item.arguments, then assert that the projected data.files array contains the expected derived file entry. Keep the existing toolName, input, result, and payload-size assertions intact while covering the projectMcpToolCallData files branch.Source: Coding guidelines
apps/server/src/orchestration/ActivityPayloadProjection.ts (2)
434-466: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueConsider storing only the last completion index per key.
The predicate on line 465 asks whether any completion index exceeds
index. That is equivalent to comparing against the largest completion index for the key. Storing a single number removes the innersomescan and the array allocations.The current form is correct. This is a simplification, not a fix.
♻️ Proposed simplification
- const completionIndicesByKey = new Map<string, number[]>(); + const lastCompletionIndexByKey = new Map<string, number>(); for (let index = 0; index < activities.length; index += 1) { const activity = activities[index]!; if (activity.kind !== "tool.completed") { continue; } const identity = toolLifecycleIdentity(activity); if (!identity) { continue; } - const key = `${activity.turnId ?? ""} ${identity}`; - const indices = completionIndicesByKey.get(key); - if (indices) { - indices.push(index); - } else { - completionIndicesByKey.set(key, [index]); - } + // Indices increase monotonically, so the last write is the largest. + lastCompletionIndexByKey.set(`${activity.turnId ?? ""} ${identity}`, index); } - if (completionIndicesByKey.size === 0) { + if (lastCompletionIndexByKey.size === 0) { return activities; } return activities.filter((activity, index) => { if (activity.kind !== "tool.updated") { return true; } const identity = toolLifecycleIdentity(activity); if (!identity) { return true; } - const indices = completionIndicesByKey.get(`${activity.turnId ?? ""} ${identity}`); - return !indices?.some((completionIndex) => completionIndex > index); + const lastCompletionIndex = lastCompletionIndexByKey.get( + `${activity.turnId ?? ""} ${identity}`, + ); + return lastCompletionIndex === undefined || lastCompletionIndex <= index; });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/server/src/orchestration/ActivityPayloadProjection.ts` around lines 434 - 466, Simplify completion tracking in the activity projection by changing completionIndicesByKey to store only the latest completion index for each tool-lifecycle key. Update the collection loop to overwrite the key with the current index, and adjust the tool.updated filter predicate to compare that single index directly instead of scanning an array with some.
225-229: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider extracting the changed-files block.
Lines 225-229 duplicate lines 294-299 exactly. Both call
collectChangedFilesover the rawdataand map the result to{ path }records. A small helper keeps the two branches in sync if the bound or the mapped shape changes later.Guidelines require extracting shared logic instead of duplicating local implementations.
♻️ Proposed helper extraction
+function projectChangedFiles(data: Record<string, unknown>): Array<{ path: string }> | undefined { + const changedFiles: string[] = []; + // Both clients discover file names by walking objects with path-like keys. + collectChangedFiles(data, changedFiles, new Set<string>(), 0); + return changedFiles.length > 0 ? changedFiles.map((path) => ({ path })) : undefined; +}Then in
projectMcpToolCallData:- const changedFiles: string[] = []; - collectChangedFiles(data, changedFiles, new Set<string>(), 0); - if (changedFiles.length > 0) { - projectedData.files = changedFiles.map((path) => ({ path })); - } + const files = projectChangedFiles(data); + if (files) { + projectedData.files = files; + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/server/src/orchestration/ActivityPayloadProjection.ts` around lines 225 - 229, Extract the duplicated changed-files collection and mapping logic into a shared helper near the projection utilities, reusing collectChangedFiles and returning the { path } records. Replace the local blocks in both projectMcpToolCallData and the other affected projection branch with this helper while preserving the existing empty-result behavior.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/server/src/server.ts`:
- Around line 563-590: Add focused lifecycle tests around the
cleanupBeforeActivation branch in the server startup flow, using
ServerActivation gates to cover both pendingServiceUpdateExists=true (finalizer
registered before forkParked activation) and false (finalizer registered after
activation). Assert that managed-tunnel release occurs at the appropriate
shutdown timing for each path, while preserving existing
releaseManagedTunnelOnShutdown coverage.
In `@apps/server/src/serviceLauncher.test.ts`:
- Around line 115-121: Update the stop-marker regression test around
launcher.stop("SIGTERM") to avoid asserting after yielding to the scheduler.
Start a child during recovery and have its shutdown handler verify that
SERVICE_STOP_MARKER_FILE exists, then await the stopping and running effects
while preserving the existing cleanup assertions.
In `@apps/server/src/serviceLauncher.ts`:
- Around line 352-355: In apps/server/src/serviceLauncher.ts lines 352-355,
update `#recover`() to remove the stop marker only when `#stopRequested` is false,
preserving it during explicit shutdown. In
apps/server/src/serviceLauncher.test.ts lines 115-121, add coverage that starts
a child through recovery, stops it, and verifies the child’s shutdown handler
observes the marker.
In `@apps/server/test/ActivityPayloadProjection.test.ts`:
- Around line 371-388: Update the anonymous fixture in projectedIds to use a
schema-valid non-empty summary instead of the whitespace-only value, while
preserving the existing identity-less row behavior and assertions.
In `@apps/web/src/components/ServerUpdateAction.tsx`:
- Line 75: Update the action-table entry in docs/user/updating.md from “Update
server” to “Update” to match the default label in ServerUpdateAction and its
call sites.
---
Nitpick comments:
In `@apps/server/src/orchestration/ActivityPayloadProjection.test.ts`:
- Around line 79-99: Extend the MCP projection test around
projectActivityPayload to include a path-like value in input or item.arguments,
then assert that the projected data.files array contains the expected derived
file entry. Keep the existing toolName, input, result, and payload-size
assertions intact while covering the projectMcpToolCallData files branch.
In `@apps/server/src/orchestration/ActivityPayloadProjection.ts`:
- Around line 434-466: Simplify completion tracking in the activity projection
by changing completionIndicesByKey to store only the latest completion index for
each tool-lifecycle key. Update the collection loop to overwrite the key with
the current index, and adjust the tool.updated filter predicate to compare that
single index directly instead of scanning an array with some.
- Around line 225-229: Extract the duplicated changed-files collection and
mapping logic into a shared helper near the projection utilities, reusing
collectChangedFiles and returning the { path } records. Replace the local blocks
in both projectMcpToolCallData and the other affected projection branch with
this helper while preserving the existing empty-result behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 750eaaaf-b826-4ab9-92fb-f23e2cd7cbf2
📒 Files selected for processing (29)
apps/mobile/src/features/threads/PendingApprovalCard.tsxapps/mobile/src/features/threads/PendingUserInputCard.tsxapps/server/src/cloud/bootService.test.tsapps/server/src/cloud/bootService.tsapps/server/src/cloud/http.test.tsapps/server/src/cloud/http.tsapps/server/src/cloud/serviceProtocol.tsapps/server/src/orchestration/ActivityPayloadProjection.test.tsapps/server/src/orchestration/ActivityPayloadProjection.tsapps/server/src/provider/acp/AcpCoreRuntimeEvents.test.tsapps/server/src/provider/acp/AcpCoreRuntimeEvents.tsapps/server/src/server.tsapps/server/src/serviceLauncher.test.tsapps/server/src/serviceLauncher.tsapps/server/test/ActivityPayloadProjection.test.tsapps/web/src/components/ChatView.tsxapps/web/src/components/ComposerPromptEditor.tsxapps/web/src/components/ServerUpdateAction.test.tsxapps/web/src/components/ServerUpdateAction.tsxapps/web/src/components/Sidebar.snooze.test.tsapps/web/src/components/Sidebar.snooze.tsapps/web/src/components/SidebarV2.tsxapps/web/src/components/chat/runtimeModePresentation.tsapps/web/src/components/composerInlineChip.tsapps/web/src/components/settings/ConnectionsSettings.tsxapps/web/src/hooks/useThreadActions.tsapps/web/src/planSidebarDismissal.tsapps/web/src/session-logic.test.tsdocs/user/updating.md
| const releaseManagedTunnel = releaseManagedTunnelOnShutdown().pipe( | ||
| Effect.timeout("10 seconds"), | ||
| Effect.tap((released) => | ||
| released ? Effect.logInfo("Released the managed tunnel on shutdown") : Effect.void, | ||
| ), | ||
| Effect.catchCause((cause) => | ||
| Effect.logWarning( | ||
| "Failed to release the managed tunnel on shutdown; the next link reuses it", | ||
| { cause }, | ||
| ), | ||
| ), | ||
| Effect.asVoid, | ||
| ); | ||
| // A launcher trial can be stopped before activation. The previous | ||
| // server is already gone, so the trial owns cleanup immediately; the | ||
| // pending-state check keeps the tunnel for normal commit or rollback, | ||
| // while the launcher's explicit-stop marker allows it to be released. | ||
| // Other runtimes wait for activation so a failed standby cannot tear | ||
| // down the active runtime's tunnel. | ||
| const cleanupBeforeActivation = yield* pendingServiceUpdateExists; | ||
| if (cleanupBeforeActivation) { | ||
| yield* Effect.addFinalizer(() => releaseManagedTunnel); | ||
| } | ||
| yield* forkParked( | ||
| Effect.gen(function* () { | ||
| // Only an activated runtime owns the tunnel cleanup finalizer. | ||
| yield* Effect.addFinalizer(() => | ||
| releaseManagedTunnelOnShutdown().pipe( | ||
| Effect.timeout("10 seconds"), | ||
| Effect.tap((released) => | ||
| released | ||
| ? Effect.logInfo("Released the managed tunnel on shutdown") | ||
| : Effect.void, | ||
| ), | ||
| Effect.catchCause((cause) => | ||
| Effect.logWarning( | ||
| "Failed to release the managed tunnel on shutdown; the next link reuses it", | ||
| { cause }, | ||
| ), | ||
| ), | ||
| Effect.asVoid, | ||
| ), | ||
| ); | ||
| if (!cleanupBeforeActivation) { | ||
| yield* Effect.addFinalizer(() => releaseManagedTunnel); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Node version:"
node -v || true
echo "Relevant files:"
git ls-files | rg 'apps/server/src/(server|.*test|.*spec).*\.(ts|tsx)$|server.ts$|test' || true
echo "server outline:"
ast-grep outline apps/server/src/server.ts --view compact | sed -n '1,220p' || true
echo "Search releaseManagedTunnelOnShutdown and cleanupBeforeActivation:"
rg -n "releaseManagedTunnelOnShutdown|cleanupBeforeActivation|pendingServiceUpdateExists|forkParked|addFinalizer|shutdown" apps/server/src/server.tsRepository: aaditagrawal/t3code
Length of output: 50376
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Git status/stat:"
git status --short || true
echo "Target server.ts lines:"
sed -n '520,610p' apps/server/src/server.ts
echo "Search for server.ts tests/specs nearby:"
git ls-files apps/server/src | rg '(^|/)server.*\.(test|spec)\.(ts|tsx)$|server\.test|server\.spec' || true
echo "Repository-wide focused search for server lifecycle cleanup terms:"
rg -n "addFinalizer|shutdown|cleanupBeforeActivation|pendingServiceUpdateExists|releaseManagedTunnelOnShutdown|forkParked" apps/server/src --glob '*.ts' --glob '*.tsx' || true
echo "List apps/server/src files with tests:"
git ls-files apps/server/src | rg '/src/.*\.(test|spec)\.(ts|tsx)$' | head -200 || trueRepository: aaditagrawal/t3code
Length of output: 44346
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "cloud http tests around releaseManagedTunnelOnShutdown:"
sed -n '36,520p' apps/server/src/cloud/http.test.ts
echo "serverActivation.ts:"
cat -n apps/server/src/serverActivation.ts
echo "serverActivation tests:"
sed -n '1,120p' apps/server/src/serverActivation.test.ts
echo "serverLifecycle/servers tests:"
sed -n '1,260p' apps/server/src/serverLifecycleEvents.test.ts
sed -n '1,260p' apps/server/src/serverActivation.test.ts || trueRepository: aaditagrawal/t3code
Length of output: 23880
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "serverRuntimeStartup relevant lines:"
sed -n '330,380p' apps/server/src/serverRuntimeStartup.ts
echo "Tests referencing ServerActivation/forkParked:"
rg -n "ServerActivation|forkParked|pendingServiceUpdateExists|releaseManagedTunnel|addFinalizer" apps/server/src --glob '*.test.ts' --glob '*.test.tsx' || true
echo "Read-only structural probe for finalizer placement invariant from source text:"
python3 - <<'PY'
from pathlib import Path
import re
server = Path("apps/server/src/server.ts").read_text()
m = re.search(r"const cleanupBeforeActivation = .*?\n\s+if \(cleanupBeforeActivation\) \{\n\s+yield\* Effect\.addFinalizer\(\(\) => releaseManagedTunnel\);\n\s+\}\n\s+yield\* forkParked\(\n\s+Effect\.gen\(function\* \(\) \{\n\s+if \(!cleanupBeforeActivation\) \{\n\s+yield\* Effect\.addFinalizer\(\(\) => releaseManagedTunnel\);\n\s+\}", server)
print("finalizer_placement_pattern_found=", bool(m))
if m:
print("match_line=", server[:m.start()].count("\n") + 1, "-", server[:m.end()].count("\n") + 1)
else:
print("nearest_patterns:")
for term in ["const cleanupBeforeActivation", "yield* Effect.addFinalizer(() => releaseManagedTunnel)", "if (!cleanupBeforeActivation)"]:
idx = server.find(term)
print(term, "line", server[:idx].count("\n") + 1 if idx >= 0 else None)
PY
echo "Tests referencing server.ts lifecycle/finalizer path:"
rg -n "finalizer|cleanupBeforeActivation|pendingServiceUpdateExists|reconcileDesiredCloudLink|forkParked" apps --glob '*.test.ts' --glob '*.test.tsx' --glob '*.test.js' --glob '*.test.jsx' || trueRepository: aaditagrawal/t3code
Length of output: 10386
Add focused lifecycle coverage for managed tunnel finalizer placement.
releaseManagedTunnelOnShutdown() has coverage, but the lifecycle code path adds the finalizer either before forkParked(activation) when pendingServiceUpdateExists is true or after activation otherwise. Add focused tests that exercise both shutdown timings with ServerActivation gates.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/server/src/server.ts` around lines 563 - 590, Add focused lifecycle
tests around the cleanupBeforeActivation branch in the server startup flow,
using ServerActivation gates to cover both pendingServiceUpdateExists=true
(finalizer registered before forkParked activation) and false (finalizer
registered after activation). Assert that managed-tunnel release occurs at the
appropriate shutdown timing for each path, while preserving existing
releaseManagedTunnelOnShutdown coverage.
Source: Coding guidelines
| it("keeps identity-less rows the clients never collapse", () => { | ||
| const anonymous: OrchestrationThreadActivity = { | ||
| id: EventId.make("upd-anon"), | ||
| tone: "tool", | ||
| kind: "tool.updated", | ||
| summary: " ", | ||
| payload: { data: { toolName: "Edit" } }, | ||
| turnId: TurnId.make("turn-a"), | ||
| createdAt: "2026-07-27T00:00:00.000Z", | ||
| }; | ||
| const completed: OrchestrationThreadActivity = { | ||
| ...anonymous, | ||
| id: EventId.make("done-anon"), | ||
| kind: "tool.completed", | ||
| }; | ||
|
|
||
| expect(projectedIds([anonymous, completed])).toEqual([anonymous.id, completed.id]); | ||
| }); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect the TrimmedNonEmptyString definition and how summary is branded.
fd -t f 'orchestration.ts' packages/contracts/src | while IFS= read -r f; do
rg -n -C 5 'TrimmedNonEmptyString' "$f" | head -60
done
# Locate the schema primitive definition.
rg -n -C 8 'TrimmedNonEmptyString\s*=' packages/contracts/src
# Check whether other tests construct summary via a constructor or a cast.
rg -n -C 3 'summary:' apps/server/test/ActivityPayloadProjection.test.ts \
apps/server/src/orchestration/ActivityPayloadProjection.test.ts
# Find producers of thread activities to see whether an empty summary is reachable.
ast-grep run --pattern 'summary: $_' --lang typescript apps/server/src/orchestration | head -40Repository: aaditagrawal/t3code
Length of output: 6771
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate ActivityPayloadProjection.ts =="
fd -t f 'ActivityPayloadProjection.ts' apps/server/src/orchestration
echo "== outline ActivityPayloadProjection.ts =="
ast-grep outline apps/server/src/orchestration/ActivityPayloadProjection.ts --view expanded | head -220 || true
echo "== summary-related code in ActivityPayloadProjection.ts =="
rg -n -C 4 'toolLifecycleIdentity|dropSupersededToolUpdatedActivities|TrimmedNonEmptyString|summary' apps/server/src/orchestration/ActivityPayloadProjection.ts
# Search for function signatures that may accept summary as bare string / schema input.
python3 - <<'PY'
from pathlib import Path
p=Path('apps/server/src/orchestration/ActivityPayloadProjection.ts')
text=p.read_text()
for needle in ['function ', 'const ', ': ']:
pass
PY
echo "== package tsconfig node version relevant setting =="
for f in package.json apps/server/package.json; do [ -f "$f" ] && { echo "--- $f"; jq '.engines' "$f" 2>/dev/null || sed -n '1,80p' "$f"; }; doneRepository: aaditagrawal/t3code
Length of output: 1698
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== ActivityPayloadProjection sections =="
sed -n '260,455p' apps/server/src/orchestration/ActivityPayloadProjection.ts
echo "== orchestration.ts summary imports/exports =="
rg -n -C 4 'OrchestrationThreadActivity|summary|TrimmedNonEmptyString|TrimmedString' packages/contracts/src/orchestration.ts
echo "== any bare string summary assignments in apps/server sources =="
python3 - <<'PY'
from pathlib import Path
for p in Path('apps/server/src').rglob('*.ts'):
text=p.read_text(errors='ignore')
if 'summary:' in text:
for i,line in enumerate(text.splitlines(),1):
if '\"summary\":' in line or 'summary:' in line:
print(f"{p}:{i}:{line.strip()}")
PYRepository: aaditagrawal/t3code
Length of output: 36676
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== ProjectionPipeline summary producer section =="
sed -n '1045,1085p' apps/server/src/orchestration/Layers/ProjectionPipeline.ts
echo "== ActivityPayloadProjection.ts full import/function boundary =="
sed -n '1,85p' apps/server/src/orchestration/ActivityPayloadProjection.ts
sed -n '455,505p' apps/server/src/orchestration/ActivityPayloadProjection.ts
echo "== read-only model check for summary: literal in fixture =="
python3 - <<'PY'
from pathlib import Path
p=Path("apps/server/test/ActivityPayloadProjection.test.ts")
text=p.read_text()
needle='summary: " "', found="summary:" in text
print(f"fixture_summary_literal={'summary: \" \"' in text}")
print(f"fixture_summary_type_annotation='summary:' in needle")
PY
echo "== type export for OrchestrationThreadActivity and Schema inference =="
rg -n -C 2 'export type OrchestrationThreadActivity|export const OrchestrationThreadActivity' packages/contracts/src/orchestration.ts
sed -n '1,22p' packages/contracts/src/baseSchemas.tsRepository: aaditagrawal/t3code
Length of output: 5069
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== base schema =="
sed -n '1,20p' packages/contracts/src/baseSchemas.ts
echo "== orchestration activity type assignment =="
sed -n '315,326p' packages/contracts/src/orchestration.ts
sed -n '1090,1120p' packages/contracts/src/orchestration.ts
sed -n '1205,1210p' packages/contracts/src/orchestration.ts
echo "== fixture literal syntax =="
python3 - <<'PY'
from pathlib import Path
p = Path("apps/server/test/ActivityPayloadProjection.test.ts")
text = p.read_text()
print('summary: " " in fixture:', 'summary: " "' in text)
PY
# Node can run a read-only behavioral probe of the relevant asTrimmedString logic
# without importing repository code.
node --input-type=module - <<'JS'
function asTrimmedString(value) {
if (typeof value !== "string") return null;
const trimmed = value.trim();
return trimmed.length > 0 ? trimmed : null;
}
console.log({
bareSpaceInput: JSON.stringify(asTrimmedString(" ")),
nonEmptySpaceInput: JSON.stringify(asTrimmedString("Edit")),
});
JSRepository: aaditagrawal/t3code
Length of output: 3113
Use a schema-valid summary in this fixture.
summary is a TrimmedNonEmptyString; " " trims to empty and violates OrchestrationThreadActivity. This literal will not pass the activity contract.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/server/test/ActivityPayloadProjection.test.ts` around lines 371 - 388,
Update the anonymous fixture in projectedIds to use a schema-valid non-empty
summary instead of the whitespace-only value, while preserving the existing
identity-less row behavior and assertions.
CodeRabbit caught that queued #recover() cleared SERVICE_STOP_MARKER_FILE after stop() wrote it, so a child started mid-shutdown could miss the explicit-stop signal. Skip marker removal when #stopRequested, assert the marker survives recover, and align updating.md with the Update button label. Co-authored-by: aaditagrawal <aaditagrawal@users.noreply.github.com>
|
Addressed CodeRabbit feedback on this tip (
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ff2218f614
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const acknowledgeActiveThreadWoke = useCallback(() => { | ||
| if (activeThreadRef === null || activeThreadWokeAt === null) return; | ||
| markThreadVisited(scopedThreadKey(activeThreadRef), activeThreadWokeAt); |
There was a problem hiding this comment.
When a previously visited thread completes, hasUnseenCompletion marks it as unread until threadLastVisitedAtById advances past the completion. This change removes the effect that advanced that timestamp whenever the completed thread was opened or updated, while the replacement here only advances it for a snooze wake during explicit actions. Consequently, opening—or already viewing—a normally completed thread no longer clears its “Done” indicator, and there is no other read action for that indicator. Keep the ordinary visit acknowledgement separate from the explicit wake acknowledgement.
Useful? React with 👍 / 👎.
| const indices = completionIndicesByKey.get(`${activity.turnId ?? ""} ${identity}`); | ||
| return !indices?.some((completionIndex) => completionIndex > index); |
There was a problem hiding this comment.
When parallel calls in one turn lack data.toolCallId and share the fallback item type/title/detail, a completion for just one call is indexed under the same key as every call. If another lifecycle row interleaves the calls, the clients would retain the still-running call's update, but this filter drops all earlier matching updates merely because one matching completion exists later. After reconnecting from a snapshot, the still-running call can therefore disappear until it emits another event; matching must use a per-call identifier or avoid dropping ambiguous fallback identities.
AGENTS.md reference: AGENTS.md:L20-L25
Useful? React with 👍 / 👎.
| default: | ||
| return "unknown"; | ||
| return "dynamic_tool_call"; |
There was a problem hiding this comment.
For ACP permission kinds such as search, fetch, or future provider-defined kinds, this now emits dynamic_tool_call, but the mobile client's requestKindFromRequestType does not recognize that type. Its derivePendingApprovals consequently discards the request, so a mobile user sees no approval card and cannot unblock the ACP session, whereas the web client explicitly maps this type to a command approval. Add the shared/mobile mapping as part of changing the emitted request type.
AGENTS.md reference: AGENTS.md:L20-L25
Useful? React with 👍 / 👎.
| const stopping = yield* fs | ||
| .exists(path.join(runtimeDir, SERVICE_STOP_MARKER_FILE)) | ||
| .pipe(Effect.orElseSucceed(() => false)); | ||
| return !stopping; |
There was a problem hiding this comment.
When an existing background-service installation is remotely updated to this server version, its stable launcher and systemd unit are not replaced, and the launcher protocol remains version 2; those pre-change launchers never write .service-stopping. If systemctl stop or t3 service uninstall occurs while an update is pending, this check therefore treats the explicit stop as a handoff and skips managed-tunnel deletion even though no replacement server is coming, leaving the provisioned tunnel orphaned. The handoff decision needs backward-compatible launcher detection or a launcher/protocol migration before absence of the marker can mean handoff.
AGENTS.md reference: AGENTS.md:L20-L25
Useful? React with 👍 / 👎.
| const indices = completionIndicesByKey.get(`${activity.turnId ?? ""} ${identity}`); | ||
| return !indices?.some((completionIndex) => completionIndex > index); |
There was a problem hiding this comment.
When a long turn repeatedly invokes the same tool identity, each update scans the completion-index array from the beginning until it finds a later completion. For an alternating sequence of k updates and completions with one key, the work is 1 + 2 + … + k, making snapshot projection quadratic precisely on the large tool-heavy threads this optimization targets. Because the indices are ordered, checking the last completion index—or performing one reverse pass with a set of future completions—keeps this linear.
AGENTS.md reference: AGENTS.md:L56-L60
Useful? React with 👍 / 👎.
| <span className="min-w-0 truncate" title={state.message}> | ||
| {state.message} | ||
| </p> | ||
| ) : null} | ||
| </span> |
There was a problem hiding this comment.
When an update failure message is wider than its container, the new truncate class hides the diagnostic and exposes the full text only through the HTML title. Touch users cannot invoke that hover-only affordance, so in narrow layouts they may see only the beginning of the rollback or installation error needed to decide whether retrying is useful. The previous progress view rendered the complete error; allow this alert to wrap or provide an explicitly operable details control.
Useful? React with 👍 / 👎.
6 participants
Bottom of the 2026-08-08 upstream sync stack (L1/7).
Fork was 71 unique commits behind
upstream/mainsince ancestrya2ca89aa. This layer ports the first 18 (+ format fix).What lands
runtimeModePresentationFork deviations preserved
rateLimitThreads/RateLimitsView#5431applied inruntimeModePresentation.ts(fork extracted staticruntimeModeConfig)Verification
vp checkandvp run typecheckpass on the branch tip.Stacked below L2 (#216). Review bottom-up.
Summary by CodeRabbit
New Features
Improvements