Extract ArgyllRunner streaming loop (runStreamingTool) and collapse tool-failed errors #79

New Issue

2026-09-10T18:23:11+01:00

gronod commented

2026-09-10 18:23:11 +01:00

Summary

ArgyllRunner (Packages/ICCeryCore/Sources/ICCeryCore/Argyll/ArgyllRunner.swift, 879 lines) copies the same subscribe-before-spawn loop for every streaming tool. Extract one internal helper and stop inventing a new *Failed error case per binary.

This is the AGPL-isolation call site. Behaviour must not change: ARGYLL_NOT_INTERACTIVE=1 stays inside ProcessManager; runners still subscribe before runStreaming; exit 0 is necessary but not sufficient — the artefact must exist.

Spec refs

docs/03-ipc-and-process-manager.md (subscribe-before-spawn, no main-actor hop per line)
docs/04-argyll-binaries.md (per-tool argv stays in *Args enums — do not merge builders)
AGENTS.md AGPL boundary
Audit of develop @ 2e8b07c

Scope

In

Private helper on ArgyllRunner, roughly:

private func runStreamingTool(
    name: String,
    id: String,
    arguments: [String],
    workingDirectory: URL?,
    flushPartialLines: Bool = false,
    onLogBatch: (@Sendable ([String]) -> Void)? = nil
) async throws -> CollectedRun

Shared requireArtefact(_ url: URL) throws -> URL (throws .missingArtefact).
Rewrite these wrappers to call the helper (keep public signatures):
- runTargen → require basename.ti1
- runPrinttarg → helper, then existing manifest + TiffPreview post-pass
- runAverage → require basename.ti3
- runColprof → helper with flushPartialLines: true, then ArtefactProbe.resolveProfile
- runIccgamut → require stem.gam
- runProfcheck → helper, then ProfcheckParser
- runCalibrationTargen → require CAL_*.ti1
Captured tools (runApplycal, runPrintcal) stay captured. Optionally share ensureNotRunning + log-split of captured stdout; do not route them through the streaming helper.
runChartread stays its own AsyncStream (prompt/row protocol). After the helper exists, reuse ensureNotRunning + batch-flush constants only.
Collapse ArgyllRunnerError:
- Keep: .missingArtefact, .malformedManifest, .instrumentDetectionFailed, .profcheckUnparseable
- Replace .processFailed / .chartreadFailed / .averageFailed / .colprofFailed / .printcalFailed / .applycalFailed / .iccgamutFailed / .profcheckFailed with:

case toolFailed(tool: String, code: Int32, logs: [String])

User-facing errorDescription stays specific ("Profile creation failed" vs "Averaging failed") via the tool string — do not regress banner copy in Stage 3/4/0.

Out

Merging TargenArgs / PrinttargArgs / ColprofArgs / etc.
Changing ProcessManager pipe/EOF contract (that is the sibling ProcessManager ticket).
View-model log hopping (sibling logged-run ticket).

Full solution

Lift the existing private collect(...) + ensureNotRunning + spawn block into runStreamingTool.
Each public runX becomes: sanitize/resolve cwd → *Args.build → ProcessID.* → binaryResolver.resolve → runStreamingTool → interpret CollectedRun.
Preserve printtarg page preview construction exactly (file-missing / TIFF-decode error strings).
Preserve colprof .icm-wins-over-.icc via ArtefactProbe.resolveProfile (#69). Do not reimplement extension swap here (sibling artefact ticket may later feed a URL-based helper; call the existing basename API for now).
runCalibrationTargen must still accept a non-prefixed stem and write CAL_{stem}.ti1. Do not invent a second prefix policy in this ticket if the CalibrationIdentity ticket has not landed — keep the current hasPrefix("CAL_") ternary and leave a // TODO(M7-cal-identity) only if you touch that line.

Rewrite invariants

Subscribe to processManager.events() before runStreaming.
Never search $PATH; resolver only.
Do not hop to @MainActor inside the runner; onLogBatch stays coalesced (20 lines or ~100 ms).
flushPartialLines remains colprof-only unless a test proves another tool needs it.
Public method names stay (runTargen, runPrinttarg, …) — UI tests and view models call these.

Dependencies

Blocks-on: none (M1–M6 closed on develop).
Unblocks: CalibrationIdentity ticket (moves CAL_ prefix out of the runner), cleanup/ArgsBuilder ticket (safer once this file shrinks).

Test

Update / add under Tests/ICCeryCoreTests/:

ArgyllRunnerColprofTests.swift, ArgyllRunnerCalibrationTests.swift — keep existing success/failure fixtures; they must still compile after the error-enum collapse. Map XCTAssertThrowsError / catch patterns from .colprofFailed to .toolFailed(tool: "colprof", ...).
New ArgyllRunnerStreamingLoopTests.swift (or extend an existing runner test):
- Helper rejects non-zero exit as .toolFailed and still returns accumulated logs.
- Helper throws .missingArtefact when exit is 0 but the expected file is absent (use a fixture binary that exits 0 and writes nothing).
- flushPartialLines: true eventually delivers a partial-line batch (colprof-dot fixture).
- Subscribe-before-spawn: a fixture that prints one line and exits immediately still delivers that line (regression for the historical lost-first-line bug).
PrinttargTests.swift / measurement tests that pattern-match ArgyllRunnerError — update exhaustiveness.
UI tests (Tests/ICCeryUITests/Milestone2UITests.swift etc.) should not need edits if public APIs and banner strings stay stable. If a banner string changes because of the error collapse, update the accessibility assertion and record the old/new copy in the PR.

Acceptance criteria

No public runX signature change.
Streaming tools share one spawn/collect implementation; wrappers are <40 lines each except printtarg and chartread.
ArgyllRunnerError no longer has per-tool *Failed duplicates.
Existing runner unit tests updated and green.
New loop tests cover non-zero exit, missing artefact, and immediate-exit log delivery.
xcodebuild test -scheme ICCery -destination 'platform=macOS' (or package test target) green on the milestone branch.

## Summary `ArgyllRunner` (`Packages/ICCeryCore/Sources/ICCeryCore/Argyll/ArgyllRunner.swift`, 879 lines) copies the same subscribe-before-spawn loop for every streaming tool. Extract one internal helper and stop inventing a new `*Failed` error case per binary. This is the AGPL-isolation call site. Behaviour must not change: `ARGYLL_NOT_INTERACTIVE=1` stays inside `ProcessManager`; runners still subscribe *before* `runStreaming`; exit 0 is necessary but not sufficient — the artefact must exist. ## Spec refs - `docs/03-ipc-and-process-manager.md` (subscribe-before-spawn, no main-actor hop per line) - `docs/04-argyll-binaries.md` (per-tool argv stays in `*Args` enums — do **not** merge builders) - `AGENTS.md` AGPL boundary - Audit of `develop` @ `2e8b07c` ## Scope **In** - Private helper on `ArgyllRunner`, roughly: ```swift private func runStreamingTool( name: String, id: String, arguments: [String], workingDirectory: URL?, flushPartialLines: Bool = false, onLogBatch: (@Sendable ([String]) -> Void)? = nil ) async throws -> CollectedRun ``` - Shared `requireArtefact(_ url: URL) throws -> URL` (throws `.missingArtefact`). - Rewrite these wrappers to call the helper (keep public signatures): - `runTargen` → require `basename.ti1` - `runPrinttarg` → helper, then existing manifest + `TiffPreview` post-pass - `runAverage` → require `basename.ti3` - `runColprof` → helper with `flushPartialLines: true`, then `ArtefactProbe.resolveProfile` - `runIccgamut` → require `stem.gam` - `runProfcheck` → helper, then `ProfcheckParser` - `runCalibrationTargen` → require `CAL_*.ti1` - Captured tools (`runApplycal`, `runPrintcal`) stay captured. Optionally share `ensureNotRunning` + log-split of captured stdout; do not route them through the streaming helper. - `runChartread` stays its own `AsyncStream` (prompt/row protocol). After the helper exists, reuse `ensureNotRunning` + batch-flush constants only. - Collapse `ArgyllRunnerError`: - Keep: `.missingArtefact`, `.malformedManifest`, `.instrumentDetectionFailed`, `.profcheckUnparseable` - Replace `.processFailed` / `.chartreadFailed` / `.averageFailed` / `.colprofFailed` / `.printcalFailed` / `.applycalFailed` / `.iccgamutFailed` / `.profcheckFailed` with: ```swift case toolFailed(tool: String, code: Int32, logs: [String]) ``` User-facing `errorDescription` stays specific (`"Profile creation failed"` vs `"Averaging failed"`) via the `tool` string — do not regress banner copy in Stage 3/4/0. **Out** - Merging `TargenArgs` / `PrinttargArgs` / `ColprofArgs` / etc. - Changing `ProcessManager` pipe/EOF contract (that is the sibling ProcessManager ticket). - View-model log hopping (sibling logged-run ticket). ## Full solution 1. Lift the existing private `collect(...)` + `ensureNotRunning` + spawn block into `runStreamingTool`. 2. Each public `runX` becomes: sanitize/resolve cwd → `*Args.build` → `ProcessID.*` → `binaryResolver.resolve` → `runStreamingTool` → interpret `CollectedRun`. 3. Preserve printtarg page preview construction exactly (file-missing / TIFF-decode error strings). 4. Preserve colprof `.icm`-wins-over-`.icc` via `ArtefactProbe.resolveProfile` (#69). Do not reimplement extension swap here (sibling artefact ticket may later feed a URL-based helper; call the existing basename API for now). 5. `runCalibrationTargen` must still accept a non-prefixed stem and write `CAL_{stem}.ti1`. Do **not** invent a second prefix policy in this ticket if the CalibrationIdentity ticket has not landed — keep the current `hasPrefix("CAL_")` ternary and leave a `// TODO(M7-cal-identity)` only if you touch that line. ## Rewrite invariants - Subscribe to `processManager.events()` before `runStreaming`. - Never search `$PATH`; resolver only. - Do not hop to `@MainActor` inside the runner; `onLogBatch` stays coalesced (20 lines or ~100 ms). - `flushPartialLines` remains colprof-only unless a test proves another tool needs it. - Public method names stay (`runTargen`, `runPrinttarg`, …) — UI tests and view models call these. ## Dependencies Blocks-on: none (M1–M6 closed on `develop`). Unblocks: CalibrationIdentity ticket (moves `CAL_` prefix out of the runner), cleanup/ArgsBuilder ticket (safer once this file shrinks). ## Test Update / add under `Tests/ICCeryCoreTests/`: - `ArgyllRunnerColprofTests.swift`, `ArgyllRunnerCalibrationTests.swift` — keep existing success/failure fixtures; they must still compile after the error-enum collapse. Map `XCTAssertThrowsError` / `catch` patterns from `.colprofFailed` to `.toolFailed(tool: "colprof", ...)`. - New `ArgyllRunnerStreamingLoopTests.swift` (or extend an existing runner test): - Helper rejects non-zero exit as `.toolFailed` and still returns accumulated logs. - Helper throws `.missingArtefact` when exit is 0 but the expected file is absent (use a fixture binary that exits 0 and writes nothing). - `flushPartialLines: true` eventually delivers a partial-line batch (colprof-dot fixture). - Subscribe-before-spawn: a fixture that prints one line and exits immediately still delivers that line (regression for the historical lost-first-line bug). - `PrinttargTests.swift` / measurement tests that pattern-match `ArgyllRunnerError` — update exhaustiveness. - UI tests (`Tests/ICCeryUITests/Milestone2UITests.swift` etc.) should **not** need edits if public APIs and banner strings stay stable. If a banner string changes because of the error collapse, update the accessibility assertion and record the old/new copy in the PR. ## Acceptance criteria - [ ] No public `runX` signature change. - [ ] Streaming tools share one spawn/collect implementation; wrappers are <40 lines each except printtarg and chartread. - [ ] `ArgyllRunnerError` no longer has per-tool `*Failed` duplicates. - [ ] Existing runner unit tests updated and green. - [ ] New loop tests cover non-zero exit, missing artefact, and immediate-exit log delivery. - [ ] `xcodebuild test -scheme ICCery -destination 'platform=macOS'` (or package test target) green on the milestone branch.

gronod added this to the M7 — Deduplicate & consolidate (develop) milestone 2026-09-10 18:23:11 +01:00

gronod added the Kind/Enhancement

Priority

High

2

Project/ICCery-v2 Feature/Architecture Feature/Backend labels 2026-09-10 18:23:11 +01:00

gronod self-assigned this 2026-09-10 18:23:11 +01:00

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

#83 Single `CalibrationIdentity` and one `.icc`/`.icm` resolver

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

#86 Dead UI, duplicated colour types, shared argv / save-panel helpers

gronod referenced a pull request that will close this issue

2026-09-10 18:48:23 +01:00

#79 Extract ArgyllRunner streaming loop onto milestone/M7-grok #87

gronod closed this issue

2026-09-10 20:22:11 +01:00

gronod referenced this issue from a commit

2026-09-10 20:22:12 +01:00

Merge #87: #79 Extract ArgyllRunner streaming loop

gronod reopened this issue

2026-09-11 10:08:37 +01:00

gronod commented

2026-09-11 10:08:43 +01:00

Reopening #79. PR #87 auto-closed #79 when merging the stacked consolidation commit d4261ba, but the explicit streaming loop contract tests and exact toolFailed error assertions required by the ticket acceptance criteria remain absent. Issue will remain open until feat/79-runner-loop-contract-tests lands in M8 Phase 6 and the full gate passes.

Reopening #79. PR #87 auto-closed #79 when merging the stacked consolidation commit d4261ba, but the explicit streaming loop contract tests and exact toolFailed error assertions required by the ticket acceptance criteria remain absent. Issue will remain open until feat/79-runner-loop-contract-tests lands in M8 Phase 6 and the full gate passes.

gronod referenced this issue from a commit

2026-09-11 12:33:20 +01:00

test(runner): complete shared streaming loop contracts (#79)

gronod referenced this issue

2026-09-11 12:33:32 +01:00

test(runner): complete shared streaming loop contracts (#79) #102

gronod referenced this issue from a commit

2026-09-11 12:41:32 +01:00

Merge pull request 'test(runner): complete shared streaming loop contracts (#79)' (#102) from feat/79-runner-loop-contract-tests into milestone/m8-consolidation

gronod commented

2026-09-11 15:15:54 +01:00

Implementation originally landed in stacked commit d4261ba via PR #87.
Completion/verification landed in PR #102 at 78ffeff.

Acceptance evidence:

[x] Shared ArgyllRunner streaming loop contracts verified (ArgyllRunnerStreamingLoopTests).

[x] Exact toolFailed(tool:code:logs:) error assertions and missingArtefact cases verified.

[x] Colprof partial progress flushing before exit verified.

[x] Removed per-tool failure error token duplicates (processFailed, printcalFailed, etc.).

[x] Public runX signatures unchanged.

Verification at milestone/m8-consolidation 891a504ee7:

targeted suites: passed (20 tests)

full universal ICCeryCoreTests: 339 passed, 0 failed

full ICCeryUITests: 29 passed, 0 failed

Closing manually after code and tests are present on the milestone branch.

Implementation originally landed in stacked commit d4261ba via PR #87. Completion/verification landed in PR #102 at 78ffeff. Acceptance evidence: [x] Shared ArgyllRunner streaming loop contracts verified (ArgyllRunnerStreamingLoopTests). [x] Exact toolFailed(tool:code:logs:) error assertions and missingArtefact cases verified. [x] Colprof partial progress flushing before exit verified. [x] Removed per-tool failure error token duplicates (processFailed, printcalFailed, etc.). [x] Public runX signatures unchanged. Verification at milestone/m8-consolidation 891a504ee722037ab15e01c1ae801a4d4cb9415a: targeted suites: passed (20 tests) full universal ICCeryCoreTests: 339 passed, 0 failed full ICCeryUITests: 29 passed, 0 failed Closing manually after code and tests are present on the milestone branch.

gronod closed this issue

2026-09-11 15:16:06 +01:00

gronod referenced this issue

2026-09-11 15:29:05 +01:00

Milestone 8 — UAT-ready hardening and consolidation (#79–#86) #104

Sign in to join this conversation.