[chetanunadkat]

What

Productionizes the Pointer Teacher — Clicky's embodied "point at the thing while teaching" loop, brought into Sutando. Voice/utterance → point_at(query) → screen capture (:7845) → Gemini vision → state/pointer-cmd.json → Sutando.app flies a bézier pointer to the target → the voice agent speaks the say line.

Design: docs/pointer-teacher-design.md · Pointer-brain decision: docs/adr/0001-pointer-teacher-brain.md

Where it lives (per the CLAUDE.md decision guide)

Piece	Home
point_at inline tool (sub-second hot path)	src/browser-tools.ts
PointerOverlayView + setupPointerOverlay/flyPointer/holdPointer	src/Sutando/main.swift
IPC (app watches via existing DispatchSource idiom)	state/pointer-cmd.json
Tracer-bullet POC kept as the reference artifact	pointer-teacher-tracer/

5 commits: 1 feat + 3 fix (Codex review rounds) + 1 docs (model risk). 13 files, +914/−4.

⚠️ Default-behavior delta (please note explicitly)

• POINTER_MODEL defaults to gemini-3-flash-preview and is env-overridable (process.env.POINTER_MODEL || …).
• This is a different model from VISION_MODEL (describe_screen). VISION_MODEL's preview→GA migration was a separate change already on main (fix(vision): migrate gemini-3.1-flash-lite preview→GA before May 25 deprecation #715, the May 25 2026 gemini-3.1-flash-lite-preview deprecation). This PR does not touch VISION_MODEL.
• The model + native [y,x] 0–1000 format + thinkingBudget:0 combo is load-bearing and POC-proven (design §4: CLAUDE.md 3 px, Search icon 1 px; gemini-3.1-flash-lite was 108 px off). Per ADR-0001 it must not be swapped without re-running the pointing POC.

Deprecation risk — checked, not assumed: gemini-3-flash-preview was verified on Google's deprecation page on 2026-05-19 — no shutdown date announced. It is a different model family from the deprecated gemini-3.1-flash-lite-preview. If Google later deprecates the -preview id, the POINTER_MODEL env override is a zero-code-change escape hatch (documented in .env.example). Re-benchmarking on a GA id is a tracked follow-up (design §5, open item 6).

Validation

• npm run typecheck (CI gate) — pass
• npm test (CI gate) — pass
• Accuracy/latency evidence is the documented grill POC (design §4) and the standalone repro under pointer-teacher-tracer/ (./run.sh "where do I commit here?"). This PR has no unit tests by design: the feature is GUI/vision/Swift (screen capture → vision model → AppKit overlay), which the tracer-bullet exercises end-to-end as the proof artifact rather than via mockable units.
• Environment: macOS 26.4.1 (arm64), Node v25.6.0.

Privacy / security

• Screen is captured only on point_at invocation — no ambient capture (the privacy property the originally-planned hotkey gate was protecting already holds; gate deferred as reversible future scope, design §6).
• No credentials added; no hardcoded keys; nothing reads outside the workspace contract.

Scope guards (deliberate, not gaps)

Vision-only (AX-first resolution is tracked open item #3); single/main display; reactive mode (no Lesson Spine — later layer). All documented in design §5/§6.

🤖 Generated with Claude Code

[liususan091219]

Reviewed feat/pointer-teacher-design HEAD — 914 LoC across 13 files (browser-tools.ts inline tool, Swift overlay in main.swift, IPC via state/pointer-cmd.json, design doc + ADR + tracer-bullet POC). Tight feature scope; the 5-commit history (1 feat + 3 fix from Codex review rounds + 1 docs) reads like a well-iterated piece of work.

Like (a lot):

• The diff carries its own audit trail — multiple inline Codex review, high/medium comments call out the bug + the fix. Concretely caught and addressed in this PR:
  • Hide-before-capture race (screencapture ignores NSWindow.sharingType → a stale on-screen pointer biases the next vision input); resolved via the {hide:true} IPC marker + 250ms grace.
  • Atomic publish using a fixed per-process temp name → could clobber on overlapping calls; resolved with per-invocation ${pid}.${Date.now()}.${random}.tmp.
  • Stale fade/hold/pulse timers from a prior point_at mutating the new marker mid-flight; resolved by a full timer-cancel + visual-state reset at the top of flyPointer.
  • didChangeScreenParametersNotification rebinds overlay frame so coord mapping survives display reconfig.
  • Failed sips -Z 1568 now falls back to the full-size capture path with a resized flag — no path where the next call could re-read a stale shrunk file.
• Decision-guide compliance (CLAUDE.md "Where does new code belong?"). The table in the PR body maps each piece to the right layer and the inline-tool placement is justified by the sub-second hot path. Tool is correctly added to BOTH inlineTools and ownerOnlyTools arrays in inline-tools.ts ([[feedback_register_all_tools]]).
• Deprecation risk handled, not hand-waved. PR body actually documents checking Google's deprecation page on 2026-05-19 for gemini-3-flash-preview — no shutdown announced, escape hatch is the POINTER_MODEL env override. That's exactly the level of evidence I want on a preview-id dependency.
• Defensive parsing in the point_at execute: JSON.parse → regex fallback → NaN/out-of-range guard. A garbage [y,x] is rejected before reaching the Swift overlay; can't fly the marker off-screen.
• Swift ts-monotonic guard is primed from on-disk state at setupPointerOverlay, so a pointer-cmd.json left over from a previous run doesn't fire on app launch.

Asks (ranked, mostly non-blocking):

1. Workspace-path drift risk (medium). POINTER_CMD_PATH = join(process.cwd(), 'state', 'pointer-cmd.json'), but the Swift side uses workspace + "/state/..." where workspace is the app's resolved workspace dir. Per CLAUDE.md "Workspace contract" the canonical resolution is $SUTANDO_WORKSPACE (or ~/.sutando/workspace default), NOT process.cwd(). On the migrated default these can disagree the same way the 2026-05-16 orphaned-DM incident did — Node writes to <repo>/state/pointer-cmd.json while the Swift app watches ~/.sutando/workspace/state/pointer-cmd.json. Should use the TS workspace-default helper (process.env.SUTANDO_WORKSPACE || join(homedir(), '.sutando', 'workspace')) instead of process.cwd().

2. pointAtTool is imported but not re-exported in inline-tools.ts. The export { describeScreenTool, clickTool, scrollAndDescribeTool, … } from './browser-tools.js'; line (line 17) does not list pointAtTool — only the next-line import does. For symmetry with describeScreenTool/clickTool (both re-exported), add to the re-export list. Today only the array-membership matters since the tool is consumed via inlineTools/ownerOnlyTools, but a future direct import { pointAtTool } from './inline-tools.js' would silently fail.

3. Resolver logic is testable in isolation — JSON.parse → regex-fallback → NaN/range gate is ~30 lines of pure logic. PR body argues "no unit tests by design" because GUI/vision/Swift, which is right for the overlay + IPC. But the resolver could carry stdlib-only tests in the #814 style (pin: missing-fence parses, fence-wrapped parses, regex-fallback path on noisy reply, NaN rejection, out-of-range rejection). Maybe 50 LoC, would catch a future regex regression.

4. Hide-then-capture race is bounded by a fixed 250ms grace, but not acked. If the DispatchSource event handler is delayed (heavy GC, disk pressure) past 250ms, the overlay is still on-screen when screencapture fires. Defense-in-depth would be a state-ack file (pointer-state.json with {visible:false, ts:...}) the Node side polls before capturing. Today's value is probably fine; flag as future hardening for self-repair.

5. Magic numbers without overrides: 8.0s hold, 0.7s fade, 60_000ms model timeout, 1568px sips resize, 250ms hide grace. None are env-overridable; tomorrow when one of them needs tuning, you'll be back in this file. Lowest-risk one to expose today is POINTER_HOLD_SECONDS since teaching pace varies per user.

6. Model timeout is 60s on the inline-tool lane. That's a long wait for a voice agent if the model hangs. describe_screen uses comparable budget (worth a grep to confirm), but on the inline-tool budget spec (sub-second target), 60s seems generous. Worth either documenting why pointer-teacher needs the long budget or aligning with describe_screen.

7. POINTER_MODEL env override is the escape hatch for the eventual preview-id deprecation, but a swap requires re-running the grill POC per ADR-0001. Worth dropping a one-liner pointer in .env.example next to POINTER_MODEL=... saying "if changing this, see pointer-teacher-tracer/run.sh to re-validate" — so the future operator who tries POINTER_MODEL=gemini-3.5-flash-ga (after deprecation) doesn't ship a 108px-off model.

Overall: ship it. The biggest substantive ask is the workspace-path drift (#1) — that's the kind of bug that fires silently after a deployment env change and is annoying to chase. The rest are polish or follow-up.

[CLAassistant]

All committers have signed the CLA.

[vidhuUC]

Cold review of feat/pointer-teacher-design.

Overall: looks good to merge. One flag and one nit, neither blocking.

Flag (non-blocking, but worth a follow-up issue)

POINTER_CMD_PATH is resolved at module load time via process.cwd():

const POINTER_CMD_PATH = join(process.cwd(), 'state', 'pointer-cmd.json');

This is consistent with how tasks/ and results/ are resolved elsewhere in the codebase, so it's fine for now. But if SUTANDO_WORKSPACE ever becomes the canonical state root (it's already the intended owner of state/), this will silently diverge. The resolveWorkspace() helper is already imported in conversation-server.ts — would be a clean follow-up to route state/pointer-cmd.json through it when that migration lands. Not a blocker.

Nit (cosmetic)

CONTEXT.md was added to the repo root — probably better as docs/pointer-teacher-context.md to stay consistent with the rest of the feature docs (docs/pointer-teacher-design.md, docs/adr/0001-...). Easy rename if the owner cares, otherwise fine.

What I checked

• publishPointerCmd atomic write/rename pattern — correct; per-invocation tmp name prevents concurrent point_at clobbering.
• Clear-before-capture (hide: true + 250ms) — necessary and well-explained; screencapture reads the raw framebuffer.
• Model/prompt selection — ADR-0001 justifies all three decisions (gemini-3-flash-preview, native [y,x] format, thinking off). Swapping any would need re-running the POC; comment says so.
• POINTER_MODEL env escape hatch — clean.
• Tracer kept in pointer-teacher-tracer/ as the proof artifact — good call.
• Design docs and ADR are thorough and match the implementation.

LGTM for merge pending owner review.

[qingyun-wu]

Filed #934 to track the resolveWorkspace() follow-up @vidhuUC flagged — captured per the LGTM-Followup-Issues convention so it doesn't dissolve into the merge diff. Not blocking — green-light on this PR as-is.

[bassilkhilo-ag2]

feat(pointer-teacher) — reviewed, strong LGTM

The AX-first → gemini-3-flash-preview native-format fallback is the right architecture: AX is free, deterministic, and ~hundreds ms; vision is the exception case for apps with sparse trees (Electron, web views). Using Gemini's native [{"point":[y,x]}] 0–1000 format rather than Clicky's raw-pixel prompt is a genuine improvement — 3 px vs 23–69 px on the same model is a meaningful regression.

What works well:

• Reusing the app's existing DispatchSource dir-watch for IPC is clean — no new socket/endpoint to manage
• point_at correctly lands as an inline tool (sub-second hot path per CLAUDE.md decision guide)
• Privacy parity with hotkey approach: screen captured only on invocation, never ambient
• POINTER_MODEL env override as the escape hatch when the preview id eventually gets a GA replacement — no code change needed

One question:
The voice trigger is purely description-based (point_at description triggers on "where do I/show me/point at/teach me"). How does Gemini distinguish that from a general "show me" information request (e.g., "show me the docs")? Is there a fallback if the model calls point_at for a query where there's no obvious screen target to point to? (e.g., the AX tree returns empty and vision returns a low-confidence point)

Otherwise: architecture is sound, docs/ADR are thorough, the preview-id longevity concern is acknowledged with a clear mitigation. Ready to ship.

[liususan091219]

Review — productionize Pointer Teacher (Lucy)

Genuinely careful PR — this is high-quality work and it shows. Things I like:

1. point_at is registered in both inlineTools and ownerOnlyTools (src/inline-tools.ts) — the dual-registration that's bitten other tools is done right here.
2. The IPC publish is hardened the right way: publishPointerCmd writes to a per-call unique temp (pid + ms + random) then renameSync — atomic, and overlapping point_at calls can't clobber each other. The prior fixed-temp-name race (cited as "Codex review, high") is genuinely closed.
3. Defensive on the hot path throughout: 8s capture timeout, 60s model timeout, AbortSignal.timeout everywhere, NaN/out-of-range point rejection before it reaches the overlay, per-invocation screenshot temp + success flag so a failed sips can't feed a stale frame to the model.
4. The stale-overlay hide-then-250ms-then-capture sequence is a sharp catch — screencapture grabs the raw framebuffer and ignores NSWindow.sharingType, so this is the only way to keep a previous pointer out of the model's input. Well reasoned.
5. The POINTER_MODEL + native-point-format + thinkingBudget:0 choices are locked with a POC-citing comment that tells the next person not to swap them without re-running the grill. That's exactly the right way to protect a load-bearing empirical choice.

One load-bearing question and two placement nits:

1. Does the IPC path actually match where Sutando.app watches? POINTER_CMD_PATH = join(process.cwd(), 'state', 'pointer-cmd.json'). The comment says "process.cwd() is the repo root for core ... which is also what the Swift app resolves as workspace." But CLAUDE.md's workspace contract says per-user mutable state (including state/) lives under $SUTANDO_WORKSPACE (default ~/.sutando/workspace/), resolved via resolveWorkspace() from workspace_default.js — and that Sutando.app's AppDelegate.workspace resolves there too, not the repo root. If core runs with cwd = repo root and Sutando.app watches ~/.sutando/workspace/state/, the rename lands in one dir and the DispatchSource watches another — the pointer never flies. Either core is actually launched with cwd = the workspace (in which case the comment is misleading and should say so), or this should resolve through resolveWorkspace() to match the Swift side. This is THE thing to confirm before merge — the whole feature is an IPC handshake and both ends must agree on the path. (VOICE_SESSION_CONTEXT_PATH / tasks/ / results/ being cited as precedent doesn't settle it — if those also use process.cwd(), they have the same latent bug, just unexercised.)

2. pointer-teacher-tracer/ at repo root. A 300-line POC tracer (swift + py + run.sh) lands as a new top-level dir. Per the CLAUDE.md decision guide a self-contained thing belongs under skills/ or docs/, not a loose root dir. Is the tracer meant to ship, or is it scaffolding that should be gitignored / moved under docs/ or skills/pointer-teacher/?

3. CONTEXT.md at repo root. It's a Pointer-Teacher domain glossary — useful, but a feature-specific doc at repo root competes with README.md/CLAUDE.md. The PR already adds docs/pointer-teacher-design.md and docs/adr/0001-*; the glossary would sit more naturally next to them in docs/.

Merge state: BEHIND — needs an update-branch.

Net: the runtime code is some of the most carefully-defended tool code I've reviewed in this repo. (1) is the only thing I'd block on — confirm the two ends of the IPC agree on the path.

— Lucy (Mac Studio bot)

[sonichi]

Cold-review pass from Sutando-Mini.

LGTM. The inline tool is tight — every hairpin the Codex/Lucy/Vidhu rounds caught is reflected in the diff:

• Overlay-clear-before-capture (publishPointerCmd({ hide: true }) + 250ms settle) — fixes the screencapture-ignores-NSWindow.sharingType bias.
• Per-invocation unique temp names for both the IPC publish and the sips downscale → no clobber on overlapping point_at calls.
• Number.isFinite + 0-1000 range guard before publish → no NaN-rocket off-screen.
• Dual registration in inlineTools + ownerOnlyTools is the correct shape per the bites we've taken on similar tools before.

Load-bearing knobs that the ADR/design protect (and the diff respects):

• POINTER_MODEL = gemini-3-flash-preview via env override (not the deprecated gemini-3.1-flash-lite-preview family — different model, separate deprecation track, verified).
• thinkingConfig: { thinkingBudget: 0 } — required, the model burns budget reasoning otherwise.
• Gemini's native {"point":[y,x]} 0-1000 format, not the Clicky raw-pixel prompt.

Nit (+1 to vidhuUC's flag, non-blocking):

• POINTER_CMD_PATH = join(process.cwd(), 'state', 'pointer-cmd.json') and publishPointerCmd's mkdirSync(join(process.cwd(), 'state'), ...) both resolve via process.cwd(). Per the workspace contract (CLAUDE.md), the canonical resolver is resolveWorkspace() from src/workspace_default.ts. Today they coincide for core (cwd == repo root == workspace), so it's correct — but if $SUTANDO_WORKSPACE ever points elsewhere this will silently write to the wrong place. Worth a tracked follow-up issue (vidhu already noted), not a blocker for v1.

Recommend merging after vidhu's workspace_default flag is filed as a follow-up issue. Nice work — the multi-round Codex polish shows in the diff.

[sonichi]

Hi @chetanunadkat — friendly heads-up: the CLA bot still shows the agreement as unsigned for this PR. Could you sign at https://cla-assistant.io/sonichi/sutando?pullRequest=889 when you get a moment? That's currently the last gate before merge (substance is well-reviewed: 4 LGTMs across Lucy, Vidhu, Bassil, and the Mini cold-review; Vidhu's only flag is tracked as a follow-up in #934).

I also just pushed an update-branch to bring this in sync with main so CI re-runs against the latest.

Thanks for the careful work on the ADR + Codex review rounds.