Repository navigation
fix: a workspace's first prompt and job are sent from any bound session #1699
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
73b9d5b
cd75c35
6a11798
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -211,6 +211,10 @@ type SessionRecord = { | |
|
|
||
| const sessions = new Map<string, SessionRecord>() | ||
|
|
||
| /** Slash commands in sessions with no record. Apart from `sessions`, so an ordinary command still | ||
| * can't evict an onboarding session, and capped the same way. */ | ||
| const untrackedCommands = new Set<string>() | ||
|
|
||
| function record(sessionID: string): SessionRecord { | ||
| const existing = sessions.get(sessionID) | ||
| if (existing) return existing | ||
|
|
@@ -264,12 +268,25 @@ function claim( | |
| * the capped map and evict genuine onboarding sessions — after which `isOnboardingSession()` | ||
| * returns false and the rest of that user's activation events are silently dropped. | ||
| * | ||
| * An untracked session has no onboarding state to protect, so skipping it loses nothing: | ||
| * `first_prompt_sent` is gated on `isOnboardingSession` anyway. | ||
| * An untracked session has no onboarding state to protect, so it goes in `untrackedCommands`. | ||
| */ | ||
| export function noteCommandSubmission(sessionID: string) { | ||
| const entry = sessions.get(sessionID) | ||
| if (entry) entry.commandSubmission = true | ||
| if (entry) { | ||
| entry.commandSubmission = true | ||
| return | ||
| } | ||
| untrackedCommands.add(sessionID) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. SUGGESTION: A leaked command flag can suppress the next typed prompt
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Kept, as R9 on the review log. A command turn aborted before its first catalog, or a queued subtask command, keeps its flag, and the next typed message that starts a loop doesn't count; the one after it does. It is the same shape as the busy-loop case fixed in cd75c35. Scoping the flag to its message would need a message id that |
||
| if (untrackedCommands.size > MAX_TRACKED_SESSIONS) { | ||
| const oldest = untrackedCommands.values().next() | ||
| if (!oldest.done) untrackedCommands.delete(oldest.value) | ||
| } | ||
| } | ||
|
|
||
| /** True (once) if an untracked session's pending user message was command-submitted. A workspace's | ||
| * first prompt is counted in any session, so it needs what the record above can't hold. */ | ||
| export function consumeUntrackedCommand(sessionID: string): boolean { | ||
| return untrackedCommands.delete(sessionID) | ||
| } | ||
|
|
||
| /** True (once) if this session's pending user message was command-submitted. */ | ||
|
|
@@ -334,5 +351,6 @@ export function resetForTest() { | |
| abandonedEmitted = false | ||
| funnelStarted = false | ||
| sessions.clear() | ||
| untrackedCommands.clear() | ||
| } | ||
| // altimate_change end | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,88 @@ | ||
| // altimate_change - new file | ||
| // | ||
| // `first_prompt_sent` and `first_job_completed` for workspaces: once per workspace per machine, from | ||
| // any session bound to that workspace. Onboarding sessions keep sending these rows as they always | ||
| // did, and mark the workspace as sent, so a machine that onboards in a linked project doesn't send | ||
| // them again from a later session. The marks don't hold back an onboarding row: its row for the job | ||
| // it selected is sent even when the workspace's row was sent before it. | ||
| // | ||
| // A job has no finish signal of its own outside `sample_setup`: a skill loads instructions and the | ||
| // agent does the work with other tools. So a job here is a turn with at least one tool call that | ||
| // didn't fail, ending `completed`, the outcome `task_outcome_signal` reports as `accepted`. It | ||
| // reports as `something_else` because nothing says which job it was. | ||
| import { createHash } from "node:crypto" | ||
| import { mkdir, writeFile } from "node:fs/promises" | ||
| import path from "node:path" | ||
| import { Global } from "@/global" | ||
| import { Log } from "@/altimate/util/log" | ||
| import { Telemetry } from "@/altimate/telemetry" | ||
| import * as OnboardingTelemetry from "@/altimate/telemetry/onboarding" | ||
| import { currentScope } from "./state" | ||
|
|
||
| const log = Log.create({ service: "workspace-first-events" }) | ||
|
|
||
| export type FirstEvent = "first_prompt_sent" | "first_job_completed" | ||
|
|
||
| /** What this process already knows is sent, so a bound turn skips the filesystem after the first. */ | ||
| const sent = new Set<string>() | ||
|
|
||
| /** | ||
| * True the one time this machine should send `event` for this workspace. | ||
| * | ||
| * A mark is a file created exclusively, under the app's state directory so nothing lands in the | ||
| * user's repository, which makes the claim atomic across the `serve` processes of several windows. | ||
| * Workspace ids are per tenant, so the tenant and API are in the key. The credential is not: a second | ||
| * person signing in on the machine is still the same machine. | ||
| * | ||
| * Nothing is claimed while telemetry is off, so a machine that opts back in can still send the row. A | ||
| * mark that can't be written claims nothing either, and the next turn tries again. The claim comes | ||
| * before the send, so a process killed between the two loses the row rather than duplicating it. | ||
| */ | ||
| export async function claim(event: FirstEvent, workspaceId: number): Promise<boolean> { | ||
| if (!Telemetry.isEnabled()) return false | ||
| const scope = await currentScope().catch(() => null) | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Minor: credentials are read before the in-memory check. Once both marks exist, every typed turn and every completed tool turn still runs
That is about 3 small fs calls per claim, and at most 2 claims per turn. It's the same order as the Not blocking: the cost is small and bounded. Safe to do in a follow-up.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed. This goes into the follow-up PR after #1672 and this one merge: a claim whose mark is already in memory returns before any credential read, either by remembering the scope with the hit or by taking the scope the turn's binding was resolved under. |
||
| if (!scope) return false | ||
| const id = createHash("sha256") | ||
| .update(JSON.stringify([scope.tenant, scope.apiUrl, workspaceId, event])) | ||
| .digest("hex") | ||
| .slice(0, 32) | ||
| if (sent.has(id)) return false | ||
| const dir = path.join(Global.Path.state, "altimate-workspace-first-events") | ||
| // Apart from the write: only the exclusive create may answer EEXIST with "already sent". A failure | ||
| // here shows up on the write below. | ||
| await mkdir(dir, { recursive: true }).catch(() => undefined) | ||
| try { | ||
| await writeFile(path.join(dir, id), "", { flag: "wx", mode: 0o600 }) | ||
| sent.add(id) | ||
| return true | ||
| } catch (err) { | ||
| if ((err as NodeJS.ErrnoException).code === "EEXIST") sent.add(id) | ||
| else log.warn("could not record a workspace first event", { event, err: String(err) }) | ||
| return false | ||
| } | ||
| } | ||
|
|
||
| /** A typed prompt: the onboarding row and the workspace row are one row when both apply. */ | ||
| export async function promptSent(sessionID: string, workspaceId: number | undefined): Promise<void> { | ||
| const onboarding = OnboardingTelemetry.isOnboardingSession(sessionID) && OnboardingTelemetry.claimFirstPrompt(sessionID) | ||
| const inWorkspace = workspaceId !== undefined && (await claim("first_prompt_sent", workspaceId)) | ||
| if (onboarding || inWorkspace) { | ||
| await OnboardingTelemetry.emit({ type: "first_prompt_sent", workspace_id: workspaceId }, sessionID) | ||
| } | ||
| } | ||
|
|
||
| /** A turn in a workspace that ended `completed` with at least one tool call that didn't fail. */ | ||
| export async function jobCompleted(sessionID: string, workspaceId: number | undefined): Promise<void> { | ||
| if (workspaceId === undefined) return | ||
| if (await claim("first_job_completed", workspaceId)) { | ||
| await OnboardingTelemetry.emit( | ||
| { type: "first_job_completed", job: "something_else", workspace_id: workspaceId }, | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Minor: nothing tells a workspace row from an onboarding row.
A field such as Not blocking: the issue asks for these event names, and the doc names both sources. Safe to do in a follow-up.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed that nothing on the row tells the two sources apart today. Whether to add a field like
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Update: decided. The follow-up PR adds
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Correction: the field is |
||
| sessionID, | ||
| ) | ||
| } | ||
| } | ||
|
|
||
| /** Test seam: drops this process's memory of the marks, as a new process would start. */ | ||
| export function resetForTest() { | ||
| sent.clear() | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -141,6 +141,7 @@ function workspaceUseFields(sessionID: string, turn: number) { | |
| // altimate_change end | ||
| import { TraceContext } from "@/altimate/observability/trace-context" // altimate_change — client trace per turn | ||
| import * as OnboardingTelemetry from "@/altimate/telemetry/onboarding" // altimate_change — onboarding funnel | ||
| import * as WorkspaceFirstEvents from "@/altimate/workspace/first-events" // altimate_change — a workspace's first prompt and job | ||
| // altimate_change start — keyless public Zen predicate (flat module) | ||
| import { isPublicZen } from "@/provider/public-zen" | ||
| // altimate_change end | ||
|
|
@@ -1830,6 +1831,12 @@ export namespace SessionPrompt { | |
| // per-server timeout. Later catalogs keep the engine tools this request | ||
| // started with. | ||
| const boundary = turnBoundary(msgs) | ||
| // altimate_change start — a request's slash-command flag is read at its own boundary, so a command | ||
| // sent while the loop is busy clears its flag here and doesn't leave it for the next typed message. | ||
| const fromCommand = | ||
| boundary && | ||
| (OnboardingTelemetry.consumeCommandSubmission(sessionID) || OnboardingTelemetry.consumeUntrackedCommand(sessionID)) | ||
|
Comment on lines
+1836
to
+1838
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| // altimate_change end | ||
| const tools = boundary ? await WorkspaceEngine.atTurnStart(sessionID, catalog) : await catalog() | ||
| WorkspaceEngine.pinTurnTools(sessionID, boundary, tools) | ||
| if (boundary) workspaceNotice.settle(sessionID) | ||
|
|
@@ -1942,25 +1949,20 @@ export namespace SessionPrompt { | |
| // - `step === 1` (the enclosing block) is NOT "first message of the session". `step` | ||
| // is declared inside loop() and loop() runs once per user turn, so this block runs | ||
| // on every turn. `claimFirstPrompt` is what makes this once-per-session. | ||
| // - `isOnboardingSession` scopes it to the funnel. Without it, an onboarding-taxonomy | ||
| // event would fire for every session in the product — TUI, `run`, GitHub, API. | ||
| // - `consumeCommandSubmission` excludes slash commands, because the scan gate submits | ||
| // - `promptSent` scopes it to an onboarding session or a workspace not yet counted on this | ||
| // machine. Without that, the row would fire for every session in the product. | ||
| // - `fromCommand` excludes slash commands, because the scan gate submits | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| // a hidden `/onboard-connect` as an ordinary user message: it would otherwise be | ||
| // recorded as the user's first typed prompt in every fresh onboarding. | ||
| // - a real text part, because a message carrying only attachments (a dragged-in file, | ||
| // a pasted image) reached this block too and was recorded as the user's first typed | ||
| // prompt before they had typed anything. Same filter as the intent classifier above. | ||
| const fromCommand = OnboardingTelemetry.consumeCommandSubmission(sessionID) | ||
| const hasUserText = !!userMsg?.parts.some( | ||
| (p) => p.type === "text" && !p.ignored && !p.synthetic && p.text.trim().length > 0, | ||
| ) | ||
| if ( | ||
| !fromCommand && | ||
| hasUserText && | ||
| OnboardingTelemetry.isOnboardingSession(sessionID) && | ||
| OnboardingTelemetry.claimFirstPrompt(sessionID) | ||
| ) { | ||
| void OnboardingTelemetry.emit({ type: "first_prompt_sent", ...turnWorkspace }, sessionID) | ||
| // A subagent's session starts from the command's template or the model's prompt, not a typed one. | ||
| if (!fromCommand && hasUserText && !session.parentID) { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P2: Prompt for AI agents
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Kept, as R13 on the review log. A child session's user messages come from the task tool, its first prompt and any resume through |
||
| await WorkspaceFirstEvents.promptSent(sessionID, boundWorkspace?.datamateId) | ||
| } | ||
|
Comment on lines
+1964
to
1966
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| // altimate_change end | ||
| // altimate_change end — session start telemetry | ||
|
|
@@ -2625,6 +2627,9 @@ export namespace SessionPrompt { | |
| ? "abandoned" | ||
| : "completed" | ||
| // altimate_change start — emit quality signal, tool chain, and error fingerprint events | ||
| if (outcome === "completed" && toolCallCount > toolErrorCount) { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P2: A failed subtask still makes Prompt for AI agents
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Kept as disclosed residual R4. The job rule is
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Minor: a subagent's own turn claims the workspace's The first-prompt check skips sessions with a If that's intended, the doc should say so. If not: if (outcome === "completed" && toolCallCount > toolErrorCount && !session.parentID) {The parent's turn then counts through its Not blocking: it only affects attribution in an edge case. The workspace did run a working tool, and nothing is duplicated or lost.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed, the workspace row shouldn't name a session the user never typed in. This PR stays as approved, and the fix goes into one follow-up PR after #1672 and this one merge: the gate gets |
||
| await WorkspaceFirstEvents.jobCompleted(sessionID, boundWorkspace?.datamateId) | ||
|
Comment on lines
+2630
to
+2631
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| } | ||
| Telemetry.track({ | ||
| type: "task_outcome_signal", | ||
| ...turnWorkspace, // altimate_change — this turn's workspace, not the session's current entry | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P2: The return value of
claimis ignored, so this onboarding row still sends when a bound session already sent this workspace'sfirst_job_completed. That breaks the at-most-once-per-workspace-per-machine guarantee in this order. Either document this as an intended exception infirst-events.tsand the telemetry doc, or gate the emit on the claim.Prompt for AI agents
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Intended, and now documented in cd75c35. An onboarding session sends its row for the job it selected as before, which C3 keeps unchanged. The claim only marks the workspace, so the end of that turn and later sessions do not send the workspace row as well. If the workspace row went first, the onboarding row still goes. That is disclosed residual R2 on the review log, and the header of
first-events.tsand the telemetry doc now say so.