docs(pty): correct the coverage-ignore rationale; note T-431 + windows.yml gap
A testability audit of the ignore span found the original comment overclaimed
("none of it can execute on Linux") and credited the wrong test file. Reality,
verified fragment by fragment:
- The span is excluded at FILE granularity but is not 100% syscall — _Coord /
_StartupInfoExW struct packing and write()'s empty-guard are pure transforms
that could be unit-tested on Linux if extracted from the binding-touching
methods. Tracked in T-431 (also covers the mirror gap in native_pty.dart's
POSIX marshalling).
- The pure helpers are tested by windows_pty_args_test.dart (not _test.dart).
- The FFI path's BEHAVIOUR is validated on windows-latest (real ConPTY spawn),
but windows.yml collects no coverage — so there is intentionally no line-
coverage metric for this span anywhere; correctness rests on that functional
suite + the VM soak, not on coverage.
Comment-only; no code or coverage change.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
Claude Opus 4.8
@@ -5001,3 +5001,36 @@ Sibling ConPTY-teardown fixes surfaced by the same analysis (fold in here or fil
|
||||
Progress (commit 606d3df, pre-VM hardening): two sibling quick-wins landed on the branch — cols/rows clamped to >= 2 in both PTY backends (lib/src/pty/pty_size.dart; microsoft/terminal#19922) and --timeout 60s on the dart-test pty line in ci/test.sh. Also made windows_pty.dart''s pure helpers (quoteArg / composeEnvironmentBlock / resolveExecutable) public + unit-tested off-Windows.
|
||||
|
||||
Still open and VM-gated (new/changed FFI, can''t validate off-Windows): the Job Object reaping (this ticket''s core), CancelIoEx/overlapped reader, and the close()/_closeConsole() teardown reorder. Do these in the Windows VM session and validate each with tools/windows-verify/soak-conpty.ps1 (orphan host count must go flat).', NULL, '2026-06-14 18:55:21', '2026-06-14 18:55:21', '2026-06-14 18:55:21', NULL, 'db48d5823487e2c93ecc0b1c1dd8ce25', 2) ON CONFLICT(hash) DO NOTHING;
|
||||
INSERT INTO ticket_history (ticket_record_id, field, old_value, new_value, changed_by, changed_at, created_at, updated_at, deleted_at, hash, canonical_version) VALUES ('06FCGJ30V24BJB001GZCR5QKTC', 'description', NULL, 'From the PTY testability audit (2026-06-14), prompted by the question "is any of the Windows FFI code testable on a pure I/O basis without Windows, and vice-versa?" Answer: yes on both backends, but the pure fragments are entangled with the syscall layer and need extraction before they can be unit-tested. Each claim below was adversarially verified (default-reject; rejected the rest of the proposed fragments because their output IS a syscall return, e.g. GetLastError, WriteFile byte count).
|
||||
|
||||
Context: lib/src/pty/windows_pty.dart wraps its FFI span in `// coverage:ignore-start/end` (Linux runner has no kernel32; the syscall sites are genuinely uncoverable off-Windows, and the bindings resolve through one DynamicLibrary.open so a method touching a binding can''t be entered on Linux). That exclusion is correct for the gate, but it hides a few pure transforms at file granularity. native_pty.dart (POSIX) has the mirror problem: it runs on Linux at ~78.9% but its pure marshalling is only covered incidentally by real spawns, never unit-tested.
|
||||
|
||||
## Windows (windows_pty.dart) — extract + unit-test on Linux
|
||||
Confirmed pure (verifier-approved), currently untested:
|
||||
- `_Coord` struct packing (69-74) — two clamped int16s into COORD; allocate via calloc, set x/y, read back.
|
||||
- `_StartupInfoExW` field assembly in start() (358-361) — cb / dwFlags=STARTF_USESTDHANDLES / lpAttributeList; deterministic field writes over calloc-zeroed memory.
|
||||
- `write()` empty/length guard (497-498) — returns 0 when `_dead` or `bytes.isEmpty`, before any WriteFile.
|
||||
|
||||
Plan: pull the COORD/STARTUPINFOEXW packing into free functions (e.g. `packCoord(cols, rows)`, `buildStartupInfoEx(attrList)`) that take/return plain values and don''t reference the kernel32 bindings; assert field layout in a Linux unit test. Keep the empty-guard logic in a tiny pure predicate.
|
||||
|
||||
## POSIX (native_pty.dart) — extract + unit-test directly (closes part of the 21% gap, adds gate margin)
|
||||
Confirmed pure (verifier-approved), currently only covered incidentally by integration spawns:
|
||||
- argv marshalling (222-228) — String list -> native UTF8 pointer array + null terminator.
|
||||
- envp marshalling (230-235) — Map<String,String> -> native ''KEY=VALUE'' UTF8 array.
|
||||
- write() buffer copy (406-408) — bytes[i] -> buf[i].
|
||||
- resize() Winsize init + clamp (432-435) — cols/rows -> ws.wsCol/wsRow (clamp already tested in pty_size_test).
|
||||
|
||||
Plan: extract marshalling into free helpers returning the pointer structures (inject the allocator so a test can read them back and free them); unit-test the round-trip and null-termination off any real spawn.
|
||||
|
||||
## NOT in scope (genuinely host-bound — leave excluded/uncovered)
|
||||
All the raw syscalls and anything whose output is a syscall return or that has no injection seam: CreatePipe / CreatePseudoConsole / CreateProcessW / ReadFile / WaitForSingleObject / WriteFile / ResizePseudoConsole / TerminateProcess / CloseHandle / GetLastError; the attribute-list APIs; the read/wait isolate bodies; and on POSIX the openpt/grantpt/unlockpt/ptsname + posix_spawn failure paths, EINTR/EBADF/EPIPE handling, and reader-isolate EOF reaping (~32 lines that need real OS error/timing state).
|
||||
|
||||
## Acceptance
|
||||
- New Linux unit tests for the fragments above (both backends).
|
||||
- windows_pty.dart `coverage:ignore` span narrowed to only the syscall sites (struct-packing helpers move out and are measured).
|
||||
- Coverage floor holds (or ratchets up from the added native_pty coverage).
|
||||
|
||||
## Related / separate finding (file or fold as decided)
|
||||
windows.yml runs the real ConPTY suite (start/write/resize/kill/errors) on windows-latest but collects NO coverage (no --coverage flag). So the FFI spawn path has functional validation on Windows + the VM soak (tools/windows-verify/) but no line-coverage metric anywhere. Decide whether to (a) accept functional-only validation explicitly, or (b) collect coverage on the Windows runner and merge it so the FFI path is measured. Cross-platform lcov merge is non-trivial (the gate reads one file) — may warrant a Q-record.
|
||||
|
||||
Audit detail: full per-fragment findings + adversarial verdicts in the workflow result for run wf_a3cacb2c-2c7.', NULL, '2026-06-14 22:38:01', '2026-06-14 22:38:01', '2026-06-14 22:38:01', NULL, 'd8cafbc05c3a83c1041c29ba25e4ed83', 2) ON CONFLICT(hash) DO NOTHING;
|
||||
|
||||
@@ -5601,3 +5601,37 @@ Sibling ConPTY-teardown fixes surfaced by the same analysis (fold in here or fil
|
||||
Progress (commit 606d3df, pre-VM hardening): two sibling quick-wins landed on the branch — cols/rows clamped to >= 2 in both PTY backends (lib/src/pty/pty_size.dart; microsoft/terminal#19922) and --timeout 60s on the dart-test pty line in ci/test.sh. Also made windows_pty.dart''s pure helpers (quoteArg / composeEnvironmentBlock / resolveExecutable) public + unit-tested off-Windows.
|
||||
|
||||
Still open and VM-gated (new/changed FFI, can''t validate off-Windows): the Job Object reaping (this ticket''s core), CancelIoEx/overlapped reader, and the close()/_closeConsole() teardown reorder. Do these in the Windows VM session and validate each with tools/windows-verify/soak-conpty.ps1 (orphan host count must go flat).', 'backlog', 'high', NULL, NULL, NULL, '2026-06-14 18:14:36', '2026-06-14 18:55:21', NULL, 'ef69bc22ce4d3a550b305cca44a43824', 2) ON CONFLICT(record_id) DO UPDATE SET type=excluded.type, parent_record_id=excluded.parent_record_id, title=excluded.title, description=excluded.description, status=excluded.status, priority=excluded.priority, assigned_to=excluded.assigned_to, team=excluded.team, decision_ref=excluded.decision_ref, updated_at=excluded.updated_at, deleted_at=excluded.deleted_at, hash=excluded.hash, canonical_version=excluded.canonical_version WHERE excluded.updated_at > tickets.updated_at OR (excluded.updated_at = tickets.updated_at AND excluded.hash > tickets.hash);
|
||||
INSERT INTO tickets (record_id, type, parent_record_id, title, description, status, priority, assigned_to, team, decision_ref, created_at, updated_at, deleted_at, hash, canonical_version) VALUES ('06FCGJ30V24BJB001GZCR5QKTC', 'task', NULL, 'Extract pure PTY logic from FFI methods into Linux-testable helpers; shrink coverage-ignore to raw syscalls', NULL, 'backlog', 'medium', NULL, NULL, NULL, '2026-06-14 22:37:27', '2026-06-14 22:37:27', NULL, 'b5f6cb297222325f80e292f780926a24', 2) ON CONFLICT(record_id) DO UPDATE SET type=excluded.type, parent_record_id=excluded.parent_record_id, title=excluded.title, description=excluded.description, status=excluded.status, priority=excluded.priority, assigned_to=excluded.assigned_to, team=excluded.team, decision_ref=excluded.decision_ref, updated_at=excluded.updated_at, deleted_at=excluded.deleted_at, hash=excluded.hash, canonical_version=excluded.canonical_version WHERE excluded.updated_at > tickets.updated_at OR (excluded.updated_at = tickets.updated_at AND excluded.hash > tickets.hash);
|
||||
INSERT INTO tickets (record_id, type, parent_record_id, title, description, status, priority, assigned_to, team, decision_ref, created_at, updated_at, deleted_at, hash, canonical_version) VALUES ('06FCGJ30V24BJB001GZCR5QKTC', 'task', NULL, 'Extract pure PTY logic from FFI methods into Linux-testable helpers; shrink coverage-ignore to raw syscalls', 'From the PTY testability audit (2026-06-14), prompted by the question "is any of the Windows FFI code testable on a pure I/O basis without Windows, and vice-versa?" Answer: yes on both backends, but the pure fragments are entangled with the syscall layer and need extraction before they can be unit-tested. Each claim below was adversarially verified (default-reject; rejected the rest of the proposed fragments because their output IS a syscall return, e.g. GetLastError, WriteFile byte count).
|
||||
|
||||
Context: lib/src/pty/windows_pty.dart wraps its FFI span in `// coverage:ignore-start/end` (Linux runner has no kernel32; the syscall sites are genuinely uncoverable off-Windows, and the bindings resolve through one DynamicLibrary.open so a method touching a binding can''t be entered on Linux). That exclusion is correct for the gate, but it hides a few pure transforms at file granularity. native_pty.dart (POSIX) has the mirror problem: it runs on Linux at ~78.9% but its pure marshalling is only covered incidentally by real spawns, never unit-tested.
|
||||
|
||||
## Windows (windows_pty.dart) — extract + unit-test on Linux
|
||||
Confirmed pure (verifier-approved), currently untested:
|
||||
- `_Coord` struct packing (69-74) — two clamped int16s into COORD; allocate via calloc, set x/y, read back.
|
||||
- `_StartupInfoExW` field assembly in start() (358-361) — cb / dwFlags=STARTF_USESTDHANDLES / lpAttributeList; deterministic field writes over calloc-zeroed memory.
|
||||
- `write()` empty/length guard (497-498) — returns 0 when `_dead` or `bytes.isEmpty`, before any WriteFile.
|
||||
|
||||
Plan: pull the COORD/STARTUPINFOEXW packing into free functions (e.g. `packCoord(cols, rows)`, `buildStartupInfoEx(attrList)`) that take/return plain values and don''t reference the kernel32 bindings; assert field layout in a Linux unit test. Keep the empty-guard logic in a tiny pure predicate.
|
||||
|
||||
## POSIX (native_pty.dart) — extract + unit-test directly (closes part of the 21% gap, adds gate margin)
|
||||
Confirmed pure (verifier-approved), currently only covered incidentally by integration spawns:
|
||||
- argv marshalling (222-228) — String list -> native UTF8 pointer array + null terminator.
|
||||
- envp marshalling (230-235) — Map<String,String> -> native ''KEY=VALUE'' UTF8 array.
|
||||
- write() buffer copy (406-408) — bytes[i] -> buf[i].
|
||||
- resize() Winsize init + clamp (432-435) — cols/rows -> ws.wsCol/wsRow (clamp already tested in pty_size_test).
|
||||
|
||||
Plan: extract marshalling into free helpers returning the pointer structures (inject the allocator so a test can read them back and free them); unit-test the round-trip and null-termination off any real spawn.
|
||||
|
||||
## NOT in scope (genuinely host-bound — leave excluded/uncovered)
|
||||
All the raw syscalls and anything whose output is a syscall return or that has no injection seam: CreatePipe / CreatePseudoConsole / CreateProcessW / ReadFile / WaitForSingleObject / WriteFile / ResizePseudoConsole / TerminateProcess / CloseHandle / GetLastError; the attribute-list APIs; the read/wait isolate bodies; and on POSIX the openpt/grantpt/unlockpt/ptsname + posix_spawn failure paths, EINTR/EBADF/EPIPE handling, and reader-isolate EOF reaping (~32 lines that need real OS error/timing state).
|
||||
|
||||
## Acceptance
|
||||
- New Linux unit tests for the fragments above (both backends).
|
||||
- windows_pty.dart `coverage:ignore` span narrowed to only the syscall sites (struct-packing helpers move out and are measured).
|
||||
- Coverage floor holds (or ratchets up from the added native_pty coverage).
|
||||
|
||||
## Related / separate finding (file or fold as decided)
|
||||
windows.yml runs the real ConPTY suite (start/write/resize/kill/errors) on windows-latest but collects NO coverage (no --coverage flag). So the FFI spawn path has functional validation on Windows + the VM soak (tools/windows-verify/) but no line-coverage metric anywhere. Decide whether to (a) accept functional-only validation explicitly, or (b) collect coverage on the Windows runner and merge it so the FFI path is measured. Cross-platform lcov merge is non-trivial (the gate reads one file) — may warrant a Q-record.
|
||||
|
||||
Audit detail: full per-fragment findings + adversarial verdicts in the workflow result for run wf_a3cacb2c-2c7.', 'backlog', 'medium', NULL, NULL, NULL, '2026-06-14 22:37:27', '2026-06-14 22:38:01', NULL, '43890d3049d818cea0acd681a191bc94', 2) ON CONFLICT(record_id) DO UPDATE SET type=excluded.type, parent_record_id=excluded.parent_record_id, title=excluded.title, description=excluded.description, status=excluded.status, priority=excluded.priority, assigned_to=excluded.assigned_to, team=excluded.team, decision_ref=excluded.decision_ref, updated_at=excluded.updated_at, deleted_at=excluded.deleted_at, hash=excluded.hash, canonical_version=excluded.canonical_version WHERE excluded.updated_at > tickets.updated_at OR (excluded.updated_at = tickets.updated_at AND excluded.hash > tickets.hash);
|
||||
|
||||
@@ -53,15 +53,29 @@ import 'pty_size.dart';
|
||||
// Everything from here to the `resolveExecutable` helper below is the
|
||||
// Windows-only ConPTY FFI path: Win32 structs, kernel32 bindings, and the
|
||||
// `WindowsPty` session that calls CreatePseudoConsole / CreateProcessW /
|
||||
// WaitForSingleObject. None of it can execute on the Linux CI runner that
|
||||
// produces the coverage report — there is no kernel32 to bind, so these
|
||||
// lines are structurally uncoverable off-Windows and would otherwise drag
|
||||
// the line-coverage gate down for code the gate's platform can't reach. The
|
||||
// pure, platform-agnostic spawn helpers (resolveExecutable / quoteArg /
|
||||
// composeEnvironmentBlock) sit AFTER the ignore-end below and ARE covered by
|
||||
// test/pty/windows_pty_test.dart on every platform. Behaviour of the FFI
|
||||
// path is validated on the Windows runner (windows.yml) and, end-to-end, in
|
||||
// the Windows VM soak (tools/windows-verify/).
|
||||
// WaitForSingleObject. The kernel32 symbols all resolve through one
|
||||
// `DynamicLibrary.open('kernel32.dll')` handle (below), which has no Linux
|
||||
// equivalent — so any method that touches a binding cannot even be ENTERED
|
||||
// on the ubuntu-latest runner that produces the coverage report, and the
|
||||
// syscall sites are genuinely uncoverable off-Windows.
|
||||
//
|
||||
// Honest caveat: this span is excluded at FILE granularity, but it is not
|
||||
// 100% syscall. A few fragments are pure input->output and COULD be unit-
|
||||
// tested on Linux if extracted into free helpers (the way the three pure
|
||||
// helpers below already were): the _Coord / _StartupInfoExW struct packing
|
||||
// and write()'s empty/length guard. They stay entangled here only because
|
||||
// they share a method with a kernel32 binding; T-431 tracks pulling them
|
||||
// (and the POSIX marshalling in native_pty.dart) into testable helpers and
|
||||
// shrinking this ignore span to the raw syscalls.
|
||||
//
|
||||
// The pure, platform-agnostic spawn helpers (resolveExecutable / quoteArg /
|
||||
// composeEnvironmentBlock) sit AFTER the ignore-end below and ARE covered on
|
||||
// every platform by test/pty/windows_pty_args_test.dart. The FFI path's
|
||||
// BEHAVIOUR — not its line coverage — is what the Windows runner validates:
|
||||
// windows.yml spawns real ConPTY children (start/write/resize/kill/errors)
|
||||
// but collects no coverage, so there is intentionally no line-coverage
|
||||
// metric for this span anywhere. End-to-end leak/handle correctness is
|
||||
// proven by the Windows VM soak (tools/windows-verify/).
|
||||
|
||||
// -- structs ----------------------------------------------------------------
|
||||
|
||||
|
||||
Reference in New Issue
Block a user