Stage 3 prompt stream never updates handheld UI (#19 blocker) #50

New Issue

2026-09-09T10:22:14+01:00

gronod commented

2026-09-09 10:22:14 +01:00

Summary

runChartread drops handheld prompt events, so Calibrate / Trigger / Done & Save never appear. This is why Milestone4UITests.testHandheldFixtureChartreadAndAverage is skipped. Same ticket also covers the classifier-order and Stage 3 chrome defects that will fire as soon as prompts work.

Spec refs

docs/15-stage3-chartread.md (state machine, buttons vs keys, 39 classifier tests, #93 sticky XY).
docs/05-argyll-fork.md §12.4–12.6 (real C prompt strings, matcher priority).
docs/03-ipc-and-process-manager.md (stdin independent of wait; killAll on quit; XY q\n + 500 ms + kill).
#175 Done is d. #137 send the key the prompt asks. #147/#149 park-before-kill.
Open parent: #19. Related: #20 #22.

Scope

In:

Prompt emission in ArgyllRunner.runChartread (compare previous state).
ChartreadClassifier matcher order + 39-fixture port from v0.8.5 / docs/05 §12.6.
Stage 3 button matrix: one control per state, no Skip/Undo, canFinish wired.
chartread child lifetime: cancel previous session; stream termination kills the child; app quit parks XY then killAll.
Unskip testHandheldFixtureChartreadAndAverage.

Out:

Hardware i1Pro / i1iO loop (stays on #18/#19).
ΔE formula, snapshot/average filesystem (#21).
Print / ColorSync.

Implementation notes

1. Prompt emission (root cause)

Packages/ICCeryCore/Sources/ICCeryCore/Argyll/ArgyllRunner.swift runChartread:

let classified = ChartreadClassifier.classify(line: line, previousState: state)
state = classified.state   // assigned FIRST
// ...
} else if state != previousOrContinuationState(state, classified) {
    continuation.yield(.prompt(classified))
}

previousOrContinuationState returns the new state unless the line is an XY continuation. Handheld transitions therefore compare .calibrating != .calibrating and never yield .prompt.

MeasurementWorkflowViewModel.handle only moves chartreadState on .prompt / final JSON row / .completed. Result: UI stays .idle. Calibrate never shows. Fixture test skipped as “timing.”

Fix:

let previous = state
let classified = ChartreadClassifier.classify(line: line, previousState: previous)
state = classified.state
if classified.isRemoveSheetNotice {
    continuation.yield(.removeSheetNotice)
}
if classified.sheetNumber != nil
    || classified.alignmentPatch != nil
    || classified.requestedWarningKey != nil
    || classified.state != previous
    || classified.isTableContinuation {
    continuation.yield(.prompt(classified))
}

Still emit .removeSheetNotice without forcing a state change (#93 info-only). Keep sticky TABLE_* on continuation lines.

2. Classifier priority

ChartreadClassifier treats "read strip" as awaiting-strip before the error matcher, and the error matcher accepts bare "error" / "failed".

"failed to read strip" / "error reading strip" → .awaitingStrip, not .error.

Port the 39 legacy cases + real C strings from docs/05 §12.6. Matcher order must match that table. Drop substring "read strip" as a standalone awaiting token; use the real “hit any key to read strip” / “ready to read strip” family. Error matcher: word-boundary error / failed to read / too fast / too slow; exclude no error.

3. Stage 3 chrome

Stage3View.controlButtons draws Done & Save twice on .allStripsRead and Retry twice on .error. Two btnDoneRead / btnRetry identifiers.

One button per state:

State	Button	stdin
calibrating	Calibrate	`" \n"`
awaitingStrip	Trigger	`" \n"`
tablePlaceSheet / tableAlign / promptContinue / warning	Continue (send requested key if any)	`"\n"` or `"{key}\n"`
allStripsRead	Done & Save	`"d\n"`
error	Retry	`" \n"`
running (any)	Cancel	XY: `q\n` + 500 ms + kill; else kill

No Skip / Undo. Do not also show Done & Save while .awaitingStrip in the same HStack as Trigger (Argyll accepts early d; if we keep it, give it a distinct id btnDoneReadEarly).

Wire btnFinishAndAverage to canFinish (isFinished && !passSnapshots.isEmpty), not isFinished alone. Snapshot failure must not offer Finish.

4. Child lifetime

startChartread: cancel + cancelChartread any live session for the same basename before spawn.
runChartread onTermination: cancelChartread (park if XY), do not only task.cancel().
applicationShouldTerminate: if a chartread id is live and the current instrument is XY, q\n + 500 ms then killAll(). Today it only killAll().

Optional but recommended: timeout collect / runChartread if both pipe EOFs never arrive after terminate() so a hung readabilityHandler cannot wedge Stage 3.

Rewrite invariants

#175 Done is d\n, not EOF, not mock s/u.
#137 Send the key the prompt asks (requestedWarningKey).
#93 Sticky TABLE_* ; “remove last sheet” is info-only.
#147/#149 Park XY before kill; killAll on last-window / quit.
#116 Exclusive id chartread_{basename} — a second Start must not collide with a zombie child.
#84 stdin handle independent of wait.

Dependencies

Blocks-on: #2, #18 (process id + instlist already on develop).
Unblocks: closing #19 CI/mock gate; unskips M4 UI test; makes #20 live rows reachable; #22 badge script can run.

Test

CI/mock:
- Classifier: 39+ fixtures including real C strings; "failed to read strip" → .error; "Hit [Space] to read strip A" → .awaitingStrip; "Please remove last sheet from table" preserves state + isRemoveSheetNotice.
- Prompt stream: calibration line yields .prompt(.calibrating); strip line yields .prompt(.awaitingStrip); done line yields .prompt(.allStripsRead).
- State → button matrix (one visible primary action, no Skip/Undo).
- Done → mock writes {basename}.ti3 only after d\n; snapshot then deletes canonical.
- Duplicate Start while running → exclusive-id error, first child still killable.
- Unskip testHandheldFixtureChartreadAndAverage.
Hardware: still #19 (one real strip). Do not block this ticket on it.

Acceptance criteria

Handheld fixture test is not skipped and is green.
.ti3 appears only after d\n.
No Skip/Undo controls that send s/u.
Exactly one btnDoneRead / btnRetry in the tree for a given state.
Finish & Average disabled until a pass snapshot exists.
39+ classifier fixtures land in ChartreadClassifierTests.

## Summary `runChartread` drops handheld prompt events, so Calibrate / Trigger / Done & Save never appear. This is why `Milestone4UITests.testHandheldFixtureChartreadAndAverage` is skipped. Same ticket also covers the classifier-order and Stage 3 chrome defects that will fire as soon as prompts work. ## Spec refs docs/15-stage3-chartread.md (state machine, buttons vs keys, 39 classifier tests, #93 sticky XY). docs/05-argyll-fork.md §12.4–12.6 (real C prompt strings, matcher priority). docs/03-ipc-and-process-manager.md (stdin independent of wait; killAll on quit; XY `q\n` + 500 ms + kill). #175 Done is `d`. #137 send the key the prompt asks. #147/#149 park-before-kill. Open parent: #19. Related: #20 #22. ## Scope In: - Prompt emission in `ArgyllRunner.runChartread` (compare *previous* state). - `ChartreadClassifier` matcher order + 39-fixture port from v0.8.5 / docs/05 §12.6. - Stage 3 button matrix: one control per state, no Skip/Undo, `canFinish` wired. - chartread child lifetime: cancel previous session; stream termination kills the child; app quit parks XY then `killAll`. - Unskip `testHandheldFixtureChartreadAndAverage`. Out: - Hardware i1Pro / i1iO loop (stays on #18/#19). - ΔE formula, snapshot/average filesystem (#21). - Print / ColorSync. ## Implementation notes ### 1. Prompt emission (root cause) `Packages/ICCeryCore/Sources/ICCeryCore/Argyll/ArgyllRunner.swift` `runChartread`: ```swift let classified = ChartreadClassifier.classify(line: line, previousState: state) state = classified.state // assigned FIRST // ... } else if state != previousOrContinuationState(state, classified) { continuation.yield(.prompt(classified)) } ``` `previousOrContinuationState` returns the *new* state unless the line is an XY continuation. Handheld transitions therefore compare `.calibrating != .calibrating` and never yield `.prompt`. `MeasurementWorkflowViewModel.handle` only moves `chartreadState` on `.prompt` / final JSON row / `.completed`. Result: UI stays `.idle`. Calibrate never shows. Fixture test skipped as “timing.” Fix: ```swift let previous = state let classified = ChartreadClassifier.classify(line: line, previousState: previous) state = classified.state if classified.isRemoveSheetNotice { continuation.yield(.removeSheetNotice) } if classified.sheetNumber != nil || classified.alignmentPatch != nil || classified.requestedWarningKey != nil || classified.state != previous || classified.isTableContinuation { continuation.yield(.prompt(classified)) } ``` Still emit `.removeSheetNotice` without forcing a state change (#93 info-only). Keep sticky `TABLE_*` on continuation lines. ### 2. Classifier priority `ChartreadClassifier` treats `"read strip"` as awaiting-strip *before* the error matcher, and the error matcher accepts bare `"error"` / `"failed"`. `"failed to read strip"` / `"error reading strip"` → `.awaitingStrip`, not `.error`. Port the 39 legacy cases + real C strings from docs/05 §12.6. Matcher order must match that table. Drop substring `"read strip"` as a standalone awaiting token; use the real “hit any key to read strip” / “ready to read strip” family. Error matcher: word-boundary `error` / `failed to read` / `too fast` / `too slow`; exclude `no error`. ### 3. Stage 3 chrome `Stage3View.controlButtons` draws Done & Save twice on `.allStripsRead` and Retry twice on `.error`. Two `btnDoneRead` / `btnRetry` identifiers. One button per state: | State | Button | stdin | |-------|--------|-------| | calibrating | Calibrate | `" \n"` | | awaitingStrip | Trigger | `" \n"` | | tablePlaceSheet / tableAlign / promptContinue / warning | Continue (send requested key if any) | `"\n"` or `"{key}\n"` | | allStripsRead | Done & Save | `"d\n"` | | error | Retry | `" \n"` | | running (any) | Cancel | XY: `q\n` + 500 ms + kill; else kill | No Skip / Undo. Do not also show Done & Save while `.awaitingStrip` in the same HStack as Trigger (Argyll accepts early `d`; if we keep it, give it a distinct id `btnDoneReadEarly`). Wire `btnFinishAndAverage` to `canFinish` (`isFinished && !passSnapshots.isEmpty`), not `isFinished` alone. Snapshot failure must not offer Finish. ### 4. Child lifetime - `startChartread`: cancel + `cancelChartread` any live session for the same basename before spawn. - `runChartread` `onTermination`: `cancelChartread` (park if XY), do not only `task.cancel()`. - `applicationShouldTerminate`: if a chartread id is live and the current instrument is XY, `q\n` + 500 ms then `killAll()`. Today it only `killAll()`. Optional but recommended: timeout `collect` / `runChartread` if both pipe EOFs never arrive after `terminate()` so a hung `readabilityHandler` cannot wedge Stage 3. ## Rewrite invariants #175 Done is `d\n`, not EOF, not mock `s`/`u`. #137 Send the key the prompt asks (`requestedWarningKey`). #93 Sticky TABLE_* ; “remove last sheet” is info-only. #147/#149 Park XY before kill; `killAll` on last-window / quit. #116 Exclusive id `chartread_{basename}` — a second Start must not collide with a zombie child. #84 stdin handle independent of wait. ## Dependencies Blocks-on: #2, #18 (process id + instlist already on develop). Unblocks: closing #19 CI/mock gate; unskips M4 UI test; makes #20 live rows reachable; #22 badge script can run. ## Test - CI/mock: - Classifier: 39+ fixtures including real C strings; `"failed to read strip"` → `.error`; `"Hit [Space] to read strip A"` → `.awaitingStrip`; `"Please remove last sheet from table"` preserves state + `isRemoveSheetNotice`. - Prompt stream: calibration line yields `.prompt(.calibrating)`; strip line yields `.prompt(.awaitingStrip)`; done line yields `.prompt(.allStripsRead)`. - State → button matrix (one visible primary action, no Skip/Undo). - Done → mock writes `{basename}.ti3` only after `d\n`; snapshot then deletes canonical. - Duplicate Start while running → exclusive-id error, first child still killable. - Unskip `testHandheldFixtureChartreadAndAverage`. - Hardware: still #19 (one real strip). Do not block this ticket on it. ## Acceptance criteria - [ ] Handheld fixture test is **not** skipped and is green. - [ ] `.ti3` appears only after `d\n`. - [ ] No Skip/Undo controls that send `s`/`u`. - [ ] Exactly one `btnDoneRead` / `btnRetry` in the tree for a given state. - [ ] `Finish & Average` disabled until a pass snapshot exists. - [ ] 39+ classifier fixtures land in `ChartreadClassifierTests`. - [ ]

gronod added the Kind/Bug

Priority

Critical

1

Bug/UI labels 2026-09-09 10:22:14 +01:00

gronod added a new dependency 2026-09-09 10:22:41 +01:00

#18 Instrument detection (`instlist`)

gronod added a new dependency 2026-09-09 10:22:59 +01:00

#22 XY-table flow UI

gronod added a new dependency 2026-09-09 10:23:29 +01:00

#20 Live swatch grid & ΔE₀₀

gronod commented

2026-09-09 11:07:36 +01:00

M5 follow-up — `feat/23-colprof` / PR #51

#50 body is still correct. M5 did not touch runChartread prompt emission, ChartreadClassifier, Stage3View.controlButtons, or canFinish. Handheld Calibrate / Trigger / Done still never appear. Do not close #50, #19, or M4 because Milestone 5 UI tests are green — those tests pre-stage mytarget.ti3 and skip Stage 3 entirely (Tests/ICCeryUITests/Milestone5UITests.swift).

Contradictions with the #50 body (explicit)

Scope Out: “Print / ColorSync.” Keep ColorSync print (lp / PDE / #14) out of #50. M5 does write ColorSync profile install (~/Library/ColorSync/Profiles, /Library/ColorSync/Profiles). Install defects below are out of original #50 scope. Either expand Scope In with a dated note, or file a separate M5 bug. Do not silently treat them as in-scope.
§4 child lifetime is chartread/XY-only. That was right for M4. M5 adds streaming colprof_{basename}, iccgamut_{stem}, profcheck_{ti3Path} (all via the same collect()), plus captured applycal_{stem}. Update §4: quit / last-window must killAll every child, not “if chartread and XY then q\n.” XY park remains a chartread-only prefix; then killAll().
Captured kill premature .exit was listed as “smaller.” Upgrade. M5 runApplycal treats captured exit 0 + tmp exists as success, then replaceItemAt over the real .icc. ProcessManager.kill on a captured job emits .exit with terminationStatus before the process has died (often 0). A quit or duplicate-id kill mid-applycal can replace a good profile with a truncated {input}.icc.applycal.tmp. That is data loss, not a log glitch.
collect() EOF hang was “optional.” Upgrade to required. runColprof / runIccgamut / runProfcheck all sit in collect() until exit and both pipe EOFs. colprof is the long child (cLUT, minutes, thousands of . with no newline). A hung readabilityHandler now wedges Stage 4/5, not only Stage 3. One timeout on collect() / maybeFinalize, used by every streaming runner.
Do not implement #25’s “cards show 0.00” as part of #50. #50 Scope Out already excludes the ΔE formula. M5 Stage 5 metricCard renders nil as "—", not "0.00" (Stage5View.swift). That satisfies “never silently zero.” Changing those cards to 0.00 while fixing #50 would regress #25.
#50 AC “unskip handheld fixture” is not implied by M5. testBuildProfileAndVerify is not a substitute. Leave the M4 skip in place until the prompt-stream fix is in.

Still do (unchanged from #50 body)

Compare previous ChartreadState before assigning, then yield .prompt.
Port 39 classifier fixtures; drop bare "read strip" as awaiting.
One btnDoneRead / btnRetry; wire btnFinishAndAverage to canFinish.
Cancel previous chartread_{basename} before respawn; stream onTermination must kill (park if XY).

Add to #50 (same mechanism, M5 made it worse)

A. collect() must time out and must not require both EOFs forever.
Shared by chartread, colprof, iccgamut, profcheck. If pendingExitCode is set, finalize after a short drain (e.g. 1–2 s) even if a pipe never delivers empty availableData.

B. Captured kill must not look like success.
ProcessManager.kill on captured[id]: wait for terminationHandler (or waitUntilExit) and emit that code (SIGTERM → non-zero). runApplycal must not replaceItemAt unless exitCode == 0 and tmp size ≥ 128 (same floor as installer). On any kill/cancel, delete tmp and leave the original .icc.

C. createProfile / verifyProfile / runColprof must not collide on process id.
Same exclusive-id rule as chartread (#116). Cancel+kill the live colprof_* / profcheck_* before a second Start. Today isColprofRunning gates the button; a stage switch or a stuck collect() leaves the id leased and the next Create fails with duplicateID.

D. colprof progress is line-based; the tool emits dots.
ProcessLineDecoder buffers until 0x0A. A multi-minute '.....' line never reaches ColprofProgressClassifier (only the last line of a flushed batch is classified). Not a Stage 3 bug, but it is the same decoder #50 already depends on. While touching the decoder for EOF timeout, flush a tail of . periodically or classify incomplete lines for colprof only. Do not hop @MainActor per dot (AGENTS.md).

## M5 follow-up — `feat/23-colprof` / PR #51 #50 body is **still correct**. M5 did not touch `runChartread` prompt emission, `ChartreadClassifier`, `Stage3View.controlButtons`, or `canFinish`. Handheld Calibrate / Trigger / Done still never appear. Do not close #50, #19, or M4 because Milestone 5 UI tests are green — those tests pre-stage `mytarget.ti3` and skip Stage 3 entirely (`Tests/ICCeryUITests/Milestone5UITests.swift`). ### Contradictions with the #50 body (explicit) 1. **Scope Out: “Print / ColorSync.”** Keep ColorSync *print* (`lp` / PDE / #14) out of #50. M5 **does** write ColorSync *profile install* (`~/Library/ColorSync/Profiles`, `/Library/ColorSync/Profiles`). Install defects below are **out of original #50 scope**. Either expand Scope In with a dated note, or file a separate M5 bug. Do not silently treat them as in-scope. 2. **§4 child lifetime is chartread/XY-only.** That was right for M4. M5 adds streaming `colprof_{basename}`, `iccgamut_{stem}`, `profcheck_{ti3Path}` (all via the same `collect()`), plus captured `applycal_{stem}`. **Update §4:** quit / last-window must `killAll` every child, not “if chartread and XY then `q\n`.” XY park remains a chartread-only prefix; then `killAll()`. 3. **Captured `kill` premature `.exit` was listed as “smaller.”** **Upgrade.** M5 `runApplycal` treats captured exit 0 + tmp exists as success, then `replaceItemAt` over the real `.icc`. `ProcessManager.kill` on a captured job emits `.exit` with `terminationStatus` *before* the process has died (often `0`). A quit or duplicate-id kill mid-`applycal` can replace a good profile with a truncated `{input}.icc.applycal.tmp`. That is data loss, not a log glitch. 4. **`collect()` EOF hang was “optional.”** **Upgrade to required.** `runColprof` / `runIccgamut` / `runProfcheck` all sit in `collect()` until `exit` **and** both pipe EOFs. `colprof` is the long child (cLUT, minutes, thousands of `.` with no newline). A hung `readabilityHandler` now wedges Stage 4/5, not only Stage 3. One timeout on `collect()` / `maybeFinalize`, used by every streaming runner. 5. **Do not implement #25’s “cards show 0.00” as part of #50.** #50 Scope Out already excludes the ΔE formula. M5 Stage 5 `metricCard` renders nil as `"—"`, not `"0.00"` (`Stage5View.swift`). That satisfies “never silently zero.” Changing those cards to `0.00` while fixing #50 would **regress** #25. 6. **#50 AC “unskip handheld fixture” is not implied by M5.** `testBuildProfileAndVerify` is not a substitute. Leave the M4 skip in place until the prompt-stream fix is in. --- ### Still do (unchanged from #50 body) - Compare *previous* `ChartreadState` before assigning, then yield `.prompt`. - Port 39 classifier fixtures; drop bare `"read strip"` as awaiting. - One `btnDoneRead` / `btnRetry`; wire `btnFinishAndAverage` to `canFinish`. - Cancel previous `chartread_{basename}` before respawn; stream `onTermination` must kill (park if XY). --- ### Add to #50 (same mechanism, M5 made it worse) **A. `collect()` must time out and must not require both EOFs forever.** Shared by chartread, colprof, iccgamut, profcheck. If `pendingExitCode` is set, finalize after a short drain (e.g. 1–2 s) even if a pipe never delivers empty `availableData`. **B. Captured kill must not look like success.** `ProcessManager.kill` on `captured[id]`: wait for `terminationHandler` (or `waitUntilExit`) and emit that code (`SIGTERM` → non-zero). `runApplycal` must not `replaceItemAt` unless `exitCode == 0` **and** tmp size ≥ 128 (same floor as installer). On any kill/cancel, delete tmp and leave the original `.icc`. **C. `createProfile` / `verifyProfile` / `runColprof` must not collide on process id.** Same exclusive-id rule as chartread (#116). Cancel+kill the live `colprof_*` / `profcheck_*` before a second Start. Today `isColprofRunning` gates the button; a stage switch or a stuck `collect()` leaves the id leased and the next Create fails with `duplicateID`. **D. `colprof` progress is line-based; the tool emits dots.** `ProcessLineDecoder` buffers until `0x0A`. A multi-minute `'.....'` line never reaches `ColprofProgressClassifier` (only the last line of a flushed batch is classified). Not a Stage 3 bug, but it is the same decoder #50 already depends on. While touching the decoder for EOF timeout, flush a tail of `.` periodically **or** classify incomplete lines for colprof only. Do not hop `@MainActor` per dot (AGENTS.md). ---

gronod referenced this issue

2026-09-09 11:09:24 +01:00

Milestone 5 Bugfixes #52

gronod referenced this issue from a commit

2026-09-09 13:26:03 +01:00

Fix M5 regressions and Stage 3 chartread prompt delivery (#50, #52).

gronod referenced this issue

2026-09-09 14:21:36 +01:00

M5 bugfixes — Stage 3 prompt stream, ProcessManager, profile/verification fixes (#50, #52) #53

gronod referenced this issue from a commit

2026-09-09 14:21:44 +01:00

Merge pull request 'M5 bugfixes — Stage 3 prompt stream, ProcessManager, profile/verification fixes (#50, #52)' (#53) from feat/52-m5-bugfixes into milestone/m5-profile