Repository navigation
fix: workspace telemetry follow-up: kill-switch guard, origin on first events, review fixes - #1705
ralphstodomingo wants to merge 3 commits into
Conversation
With ALTIMATE_DISABLE_WORKSPACE on, routing never reads the pin, yet a turn's rows waited up to 3 s on a validation nothing used. They now name the project's link at once, as routing does. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VEmKQswEEpkYFmMEngESx8
first_prompt_sent and first_job_completed carry origin (onboarding or workspace); rows without it are onboarding. Also: a subagent's turn is no job, a noReply attachment skips the pin wait, a turn ended early clears its verdict. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VEmKQswEEpkYFmMEngESx8
|
No actionable comments were generated in the recent review. Walkthrough
Merge Risk:
|
Review logClaims for review of this PR, numbered so a round can cite them. A finding is a reproducible trace that breaks one; the residuals are accepted trade-offs. Claims
Disclosed residuals
Rounds
Appendix — complexity delta
17 touched · 4 rose · 3 new (max cognitive 2) · net cognitive +5. The tool matches functions by line, so it paired the |
|
@codex review against the numbered claims and the disclosed residuals in the review-log comment on this PR (#1705 (comment)): report only a reproducible trace that violates a numbered claim. Instances of the disclosed residuals are not findings. A round with no claim violation ends review. |
Codex Review Summary: ✅ CompletedThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Codex Review: Didn't find any major issues. Another round soon, please! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
There was a problem hiding this comment.
4 issues found across 11 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/opencode/src/altimate/workspace/first-events.ts">
<violation number="1" location="packages/opencode/src/altimate/workspace/first-events.ts:49">
P2: `claim` falls back to ambient credentials when no scope is passed, but the onboarding hook supplies only `workspaceId`. A credential switch during the turn can mark tenant B for tenant A, after which `jobCompleted` emits a duplicate or credits the wrong tenant; pass the turn's resolved scope to this claim.</violation>
</file>
<file name="packages/opencode/src/altimate/workspace/state.ts">
<violation number="1" location="packages/opencode/src/altimate/workspace/state.ts:501">
P2: `__resetPinValidation` does not actually isolate an in-flight validation: the old `resolvePinnedBinding` continuation can still write `pinValidation` after this reset. Invalidate requests with a generation/token and ignore completions from before the reset, otherwise later tests can inherit stale pin verdicts.</violation>
</file>
<file name="packages/opencode/src/session/prompt.ts">
<violation number="1" location="packages/opencode/src/session/prompt.ts:565">
P2: `endTurnBinding` can clear a newer prompt's verdict because it is keyed only by `sessionID`. Concurrent prompts can make the later turn resolve a different workspace after its attachment already used the first; make cleanup compare against this prompt's own verdict.</violation>
</file>
<file name="packages/opencode/src/altimate/plugin/onboarding-telemetry.ts">
<violation number="1" location="packages/opencode/src/altimate/plugin/onboarding-telemetry.ts:269">
P2: `origin: "onboarding"` identifies the turn's workspace, but the suppression claim below still uses the ambient credential instead of that turn's resolved scope. Pass the resolved binding scope through the tool hook before claiming, or an account switch can mis-key the mark and produce a duplicate workspace row.</violation>
</file>
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
| const scope = await currentScope().catch(() => null) | ||
| // The credential the turn's binding was resolved under, when the caller has it: the workspace id | ||
| // belongs to that tenant, and the turn doesn't read the credentials again. | ||
| scope ??= (await currentScope().catch(() => null)) ?? undefined |
There was a problem hiding this comment.
P2: claim falls back to ambient credentials when no scope is passed, but the onboarding hook supplies only workspaceId. A credential switch during the turn can mark tenant B for tenant A, after which jobCompleted emits a duplicate or credits the wrong tenant; pass the turn's resolved scope to this claim.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At packages/opencode/src/altimate/workspace/first-events.ts, line 49:
<comment>`claim` falls back to ambient credentials when no scope is passed, but the onboarding hook supplies only `workspaceId`. A credential switch during the turn can mark tenant B for tenant A, after which `jobCompleted` emits a duplicate or credits the wrong tenant; pass the turn's resolved scope to this claim.</comment>
<file context>
@@ -38,9 +42,11 @@ const sent = new Set<string>()
- const scope = await currentScope().catch(() => null)
+ // The credential the turn's binding was resolved under, when the caller has it: the workspace id
+ // belongs to that tenant, and the turn doesn't read the credentials again.
+ scope ??= (await currentScope().catch(() => null)) ?? undefined
if (!scope) return false
const id = createHash("sha256")
</file context>
There was a problem hiding this comment.
Not changing this in this PR. It's R8 on the review log. The onboarding hook claims its mark with the credential read at claim time, as it did before this PR; C4 covers only a passed scope. Passing the turn's scope here means adding it to the tool context the plugin hook receives, which is wider than this follow-up. The effect needs a credential switch inside an onboarding turn, and it is at most one duplicate workspace row.
| export function __resetPinValidation(clock?: () => number): void { | ||
| pinValidation.clear() | ||
| // A request an earlier test started could otherwise settle after the reset and fill the memo. | ||
| pinValidationInFlight.clear() |
There was a problem hiding this comment.
P2: __resetPinValidation does not actually isolate an in-flight validation: the old resolvePinnedBinding continuation can still write pinValidation after this reset. Invalidate requests with a generation/token and ignore completions from before the reset, otherwise later tests can inherit stale pin verdicts.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At packages/opencode/src/altimate/workspace/state.ts, line 501:
<comment>`__resetPinValidation` does not actually isolate an in-flight validation: the old `resolvePinnedBinding` continuation can still write `pinValidation` after this reset. Invalidate requests with a generation/token and ignore completions from before the reset, otherwise later tests can inherit stale pin verdicts.</comment>
<file context>
@@ -496,6 +497,8 @@ let pinNow: () => number = () => Date.now()
export function __resetPinValidation(clock?: () => number): void {
pinValidation.clear()
+ // A request an earlier test started could otherwise settle after the reset and fill the memo.
+ pinValidationInFlight.clear()
pinNow = clock ?? (() => Date.now())
}
</file context>
There was a problem hiding this comment.
Partly right. The reset stops a later caller from joining a request started before it, but a caller already awaiting that request still writes the memo when it answers, so the comment overstated it. Fixed in 0909886 to say so. No code change: the tests that start a slow validation await their turn, and the turn's routing read awaits that request, so none outlives its test. A generation counter would add production state only for the test seam.
| const message = await createUserMessage(input) | ||
| // altimate_change — a prompt that fails here runs no loop, so no later loop may take its verdict | ||
| const message = await createUserMessage(input).catch((err) => { | ||
| endTurnBinding(input.sessionID) |
There was a problem hiding this comment.
P2: endTurnBinding can clear a newer prompt's verdict because it is keyed only by sessionID. Concurrent prompts can make the later turn resolve a different workspace after its attachment already used the first; make cleanup compare against this prompt's own verdict.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At packages/opencode/src/session/prompt.ts, line 565:
<comment>`endTurnBinding` can clear a newer prompt's verdict because it is keyed only by `sessionID`. Concurrent prompts can make the later turn resolve a different workspace after its attachment already used the first; make cleanup compare against this prompt's own verdict.</comment>
<file context>
@@ -560,7 +560,11 @@ export namespace SessionPrompt {
- const message = await createUserMessage(input)
+ // altimate_change — a prompt that fails here runs no loop, so no later loop may take its verdict
+ const message = await createUserMessage(input).catch((err) => {
+ endTurnBinding(input.sessionID)
+ throw err
+ })
</file context>
There was a problem hiding this comment.
Not changing this. It's R1 on the review log, now worded for any overlapping prompts. The slot has been keyed by session since #1672, including the clear at the start of prompt(). A cleared verdict makes a loop resolve again; it never takes another prompt's verdict, which is C6. Getting two answers takes a second prompt in the same session while the first is still building its message, and a validation answer that changes in between.
| // names its own turn's workspace, not the replacement's. | ||
| void OnboardingTelemetry.emit( | ||
| { type: "first_job_completed", job, workspace_id: input.workspaceId }, | ||
| { type: "first_job_completed", job, workspace_id: input.workspaceId, origin: "onboarding" }, |
There was a problem hiding this comment.
P2: origin: "onboarding" identifies the turn's workspace, but the suppression claim below still uses the ambient credential instead of that turn's resolved scope. Pass the resolved binding scope through the tool hook before claiming, or an account switch can mis-key the mark and produce a duplicate workspace row.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At packages/opencode/src/altimate/plugin/onboarding-telemetry.ts, line 269:
<comment>`origin: "onboarding"` identifies the turn's workspace, but the suppression claim below still uses the ambient credential instead of that turn's resolved scope. Pass the resolved binding scope through the tool hook before claiming, or an account switch can mis-key the mark and produce a duplicate workspace row.</comment>
<file context>
@@ -266,7 +266,7 @@ export async function OnboardingTelemetryPlugin(_input: PluginInput): Promise<Ho
// names its own turn's workspace, not the replacement's.
void OnboardingTelemetry.emit(
- { type: "first_job_completed", job, workspace_id: input.workspaceId },
+ { type: "first_job_completed", job, workspace_id: input.workspaceId, origin: "onboarding" },
input.sessionID,
)
</file context>
There was a problem hiding this comment.
Same point as the first-events.ts thread: R8 on the review log. This row's mark is claimed with the credential read at claim time, as before this PR. Carrying the turn's scope into the hook needs it on the tool context, which is out of scope here. The effect is at most one duplicate workspace row after a credential switch inside an onboarding turn.
| if (pinned) return pinned.status === "bound" ? pinned.binding : null | ||
| if (pinned) { | ||
| if (pinned.status !== "bound") return null | ||
| return { ...pinned.binding, scope: (await accountKey()) ?? undefined } |
There was a problem hiding this comment.
WARNING: The pinned arm's scope is not "the credential the binding was resolved under" — it is a second, ambient credential read taken after the async validation.
resolvePinnedBinding already resolved credentials (line 634) and used them for both the cache key and the listDatamates request, which is exactly what its own comment at ~line 624 says to do instead of also calling accountKey(). Calling accountKey() here duplicates that credential load on every pinned turn, and it re-opens the A→B window that function went to lengths to close: if the credential switches tenant/host while the pin validation is in flight, this pairs a workspace id validated under tenant A with a scope from tenant B. first-events.claim keys the mark on tenant + apiUrl + workspaceId + event, so that mismatch files the claim under the wrong tenant — suppressing a genuine first_prompt_sent/first_job_completed for B's workspace with the same id, or crediting A's row to B.
The linked arm (linkedTurnBinding) is atomic — one accountKey() read paired with the binding — so the two arms disagree on the invariant the new TurnBinding comment states. Consider returning the credential (or just tenant/apiUrl) from resolvePinnedBinding alongside the outcome instead of re-reading it here.
There was a problem hiding this comment.
Fixed in 0909886. This is the same finding cubic raised on this line. resolvePinnedBinding now returns the credential it validated under as scope on the bound verdict, and resolveTurnBinding uses it, so no credentials are read after the validation and both arms pair the binding with the credential that resolved it. A new test switches the credential mid-validation and checks the scope stays on the validating tenant. It fails without the fix.
| | `activation_job_selected` / `first_job_completed` | Which activation job the user started and, where observable, finished. In onboarding, completion is reported only for the job that was actually selected, so the two form a coherent pair. `first_job_completed` is also sent once per workspace for each machine and Altimate instance, for the first turn in a linked project that had a tool call that didn't fail and finished. **Derived** — see the note below. | | ||
| | `first_prompt_sent` | The user's first typed message in an onboarding session, and the first typed message in a linked project, once per workspace for each machine and Altimate instance. Slash commands are excluded, so the hidden `/onboard-connect` submission does not count. | | ||
| | `activation_job_selected` / `first_job_completed` | Which activation job the user started and, where observable, finished. In onboarding, completion is reported only for the job that was actually selected, so the two form a coherent pair. `first_job_completed` is also sent once per workspace for each machine and Altimate instance, for the first turn in a linked project, or in a session pinned to a workspace by the IDE extension, that had a tool call that didn't fail and ended `completed`. `origin` says which: `onboarding` or `workspace`. **Derived** — see the note below. | | ||
| | `first_prompt_sent` | The user's first typed message in an onboarding session, and the first typed message in a linked project or a session pinned to a workspace, once per workspace for each machine and Altimate instance. A slash command sent as a command, as the TUI and `run` send it, is excluded, so the hidden `/onboard-connect` submission does not count. The VS Code chat sends a command as its expanded text, so there a command counts. A subagent's prompt never counts. `origin` is `onboarding` for an onboarding session's row, bound or not, and `workspace` otherwise. | |
There was a problem hiding this comment.
SUGGESTION: This over-states origin. In first-events.ts promptSent, origin is onboarding only when isOnboardingSession(sessionID) && claimFirstPrompt(sessionID) is true — i.e. when the onboarding rule sent the row — not whenever the session is an onboarding one.
A reachable case where the two differ: an onboarding session in an unbound project sends its row on turn 1 (onboarding claim consumed, no workspace claim because workspaceId was undefined); the project is then linked mid-session; on turn 2 the onboarding claim is false and the workspace claim succeeds, so the row is emitted with origin: "workspace" even though the session is an onboarding one. The same wording is repeated in the origin comment on the first_prompt_sent variant in telemetry/index.ts (~lines 1560-1561), so both should say "whichever rule sent the row" rather than keying on the session type.
There was a problem hiding this comment.
Right, fixed in 0909886. The docs row, the note and the origin comment on the first_prompt_sent type now say which rule sent the row: onboarding for an onboarding session's first typed prompt, workspace for the workspace's row. The docs row also gives your case: a later prompt, once the project is linked, is a workspace row. C2 on the review log is reworded the same way.
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Incremental review of The two earlier findings are resolved:
Also verified: the cleaned-up Files Reviewed (4 files)
Previous Review Summary (commit 1341f81)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 1341f81)Status: 2 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (11 files)
Reviewed at Reviewed by deepseek-v4.1-flash-1017 · Input: 91.1K · Output: 20K · Cached: 1.5M Review guidance: REVIEW.md from base branch |
The pin's verdict now carries the credential it was validated under, so a switch during validation can't key the first-event mark under another tenant. Also: `origin` is described by the rule that sent the row, and a flaky timing check is dropped. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VEmKQswEEpkYFmMEngESx8
|
@codex review against the numbered claims and the disclosed residuals in the review-log comment on this PR (#1705 (comment)). Scoped round on |
|
Codex Review: Didn't find any major issues. Can't wait for the next one! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
🤖 Code Review — OpenCodeReview (Gemini) — No Issues FoundNo supported files changed. |
Issue for this PR
Closes #1671
Closes #1698
Type of change
What does this PR do?
Follow-up to #1672 and #1699, from their reviews.
ALTIMATE_DISABLE_WORKSPACEon, routing never reads the IDE extension's pin, yet a turn's rows still validated it and waited up to 3 s. Now they skip it and name the project's link, as routing does.originonfirst_prompt_sentandfirst_job_completed:onboardingorworkspace. Rows from older clients have none and are onboarding rows. Notsource: the envelope stampssourceon every row, and an event's own field would overwrite it.noReplyprompt's attachments skip the pin's validation.Claims, residuals and the complexity delta are in the review log comment.
How did you verify your code works?
Each fix has a test that fails without it, except the early-end clears and the once-only warning (R6 on the log). The tests are in
routing-pin.test.ts,first-events.test.tsandonboarding.test.ts. The prompt-loop ones are inpinned-workspace-loop.test.tsandworkspace-first-events-loop.test.ts, which now run with the workspace feature on.test/session,test/altimate/workspaceandtest/altimate/telemetrypass locally. The exceptions are two timing tests inprompt.test.ts(the exit flush, and concurrent loop callers at its 3 s limit), which fail the same way withmain's source. Typecheck, Marker Guard, branding and the tracker-leak check pass.Cost side effects
None. With the kill switch on, a turn sends one request fewer.
Screenshots / recordings
Not a UI change.
Checklist
🤖 Generated with Claude Code
https://claude.ai/code/session_01VEmKQswEEpkYFmMEngESx8
Summary by cubic
Follows up on the workspace telemetry PRs with fixes from their reviews. With
ALTIMATE_DISABLE_WORKSPACEset, a turn's rows no longer validate the IDE extension's pin or wait up to 3 s on it; they name the project's link, as routing does.first_prompt_sentandfirst_job_completednow carryorigin(onboardingorworkspace), and rows from older clients — which have none — are treated as onboarding rows. The pin's verdict now carries the credential it was validated under, so a switch during validation can't key a first-event claim under another tenant.Closes #1671 and #1698.
Smaller Fixes
taskcall counts it.noReplyprompts' attachments skip the pin's validation and wait.Written for commit 0909886. Summary will update on new commits.
Summary by CodeRabbit