Wire real tracker/notifier adapters into os-sdlc escalation #342

Closed
opened 2026-08-11 20:01:48 +00:00 by jared · 3 comments
Owner

Context

Deferred from #341 (ADR-0113 park-and-notify): EngineFactory's production call sites (plugins/os-sdlc/bin/os-sdlc-runner, hooks/post_tool_use.rb) still inject NullTracker/NullNotifier, so a parked ticket adds no waiting label, no blocker comment, and no push notification. Build a tea-based tracker port (waiting label + blocker comment) and a push-notification notifier port, and inject them at those call sites. Also retire the table-identity legacy branch (SkeletonTable .equal? checks in Resumer/StageBriefWriter) before phase 3 closes — flagged non-blocking in the #341 review.

Decomposition (2026-08-12)

Evidence gathered via os-sdlc:code-probe (ast-grep, read-only) before decomposing:

  • Port contract (confirmed, plugins/os-sdlc/lib/os_sdlc/runner/null_tracker.rb / null_notifier.rb): Tracker#add_waiting_label(ticket_id), Tracker#blocker_comment(ticket_id, message), Notifier#push(message).
  • Injection seam (plugins/os-sdlc/lib/os_sdlc/runner/engine_factory.rb, Build#tracker/Build#notifier): @options[:tracker] || NullTracker.new, @options[:notifier] || NullNotifier.new. Both production call sites never pass these options: hooks/post_tool_use.rb:56 (EngineFactory.build(root), no options at all) and bin/os-sdlc-runner:15 (EngineFactory.build(root, **options) — CLI-parsed options has no tracker/notifier flag today).
  • Legacy identity branch (plugins/os-sdlc/lib/os_sdlc/runner/stage_brief_writer.rb:22-24): legacy? => table.equal?(SkeletonTable::TABLE), gating Brief.write (old flat path) vs BriefAssembler (map-based path).
  • abort! hardcode (plugins/os-sdlc/lib/os_sdlc/runner/resumer.rb, abort!): Ticket.update_state(@context.db, @ticket[:id], "failed") — literal string, no table lookup. TransitionTable already exposes terminal_states / terminal?(state) (transition_table.rb:28,42-43), validated at load time via existing validate!.
  • No existing push-notification precedent in this repo (checked plugins/os and plugins/os-sdlc for notify-send/osascript/ntfy/pushover — none found); this is new surface, scoped minimally below.

Gate/sequencing logic stays out of scope for every subtask below (ADR-0097): none of these change routing in transition_table.rb, they add adapters and fix a data lookup.

Subtask 1 — Tea-based Tracker adapter

Add OsSdlc::Runner::TeaTracker (new file, sibling to null_tracker.rb) implementing the Tracker port: add_waiting_label(ticket_id) shells out to tea issues edit <ticket_id> --add-labels waiting --repo <configured-repo>; blocker_comment(ticket_id, message) shells out to tea comment <ticket_id> --repo <configured-repo> (or the equivalent non-hanging path — MEMORY notes tea comment hangs on large bodies; keep blocker-comment bodies short/single-line, or write via a temp file the same way this task updates #342, and verify non-interactively before landing).

  • In scope: the adapter class, its shell-out, and the repo-slug source (reuse whatever plugins/os-sdlc/lib/os_sdlc/issue_source.rb or plugins/os-backlog/lib/backlog/issues.rb already resolves for repo config — do not invent a second config path).
  • Out of scope: GitHub/gh support, retry/backoff on tea failures (a failed tracker call should not crash escalation — see Subtask 3 for the injection contract, not this subtask for resiliency policy).
  • Acceptance criteria:
    • Minitest unit test stubs the shell-out (e.g. injected command runner) and asserts add_waiting_label invokes tea issues edit <id> --add-labels waiting with the correct repo.
    • Minitest unit test asserts blocker_comment invokes the tea comment path with ticket_id and message, non-interactively (test must not actually hang or shell out to a real network call).
    • TeaTracker is a duck-type match for NullTracker: same two public method signatures, verifiable by a shared contract test module run against both.

Subtask 2 — Push notifier adapter

Add OsSdlc::Runner::PushNotifier (or similarly named, sibling to null_notifier.rb) implementing push(message). Given no existing push mechanism in this repo and the dev machine is Linux/Fedora, scope to a notify-send-based desktop notification (matches the local environment; no external service dependency).

  • In scope: the adapter class, shelling out to notify-send with the message, guarded so a missing notify-send binary degrades to a no-op-with-warning rather than raising (escalation must never crash on a notification failure — matches ADR-0113's "end the turn" framing, which assumes escalation persistence in SQLite succeeds regardless of notification delivery).
  • Out of scope: cross-platform notification backends (macOS osascript, mobile push, ntfy/Pushover integration) — no requirement or precedent for those surfaced during the probe; deferred until a concrete need appears.
  • Acceptance criteria:
    • Minitest unit test stubs the shell-out and asserts push(message) invokes notify-send with the message text.
    • Minitest unit test asserts that when the shell-out raises/fails (binary missing, non-zero exit), push swallows it without raising and the ticket's escalation persistence (already covered by existing SQLite Escalation tests) is unaffected — i.e. this subtask only needs to prove push itself doesn't raise, not re-test Escalation.
    • PushNotifier is a duck-type match for NullNotifier: same one public method signature.

Subtask 3 — Inject real adapters at both production call sites

Wire TeaTracker/PushNotifier into hooks/post_tool_use.rb:56 and plugins/os-sdlc/bin/os-sdlc-runner:15, replacing the implicit NullTracker/NullNotifier fallback for real runs. EngineFactory::Build#tracker/#notifier already read @options[:tracker]/@options[:notifier] — this subtask supplies those options at the two call sites, it does not change engine_factory.rb.

  • In scope: constructing TeaTracker.new/PushNotifier.new (with whatever config each needs, e.g. repo slug) at both call sites and passing them as EngineFactory.build(root, tracker: ..., notifier: ...).
  • Out of scope: making tracker/notifier choice configurable per-project (e.g. via .sdlc/project.yaml) — no requirement surfaced for that; both call sites get the real adapters unconditionally. If a future ticket needs a stub mode for local dev, that's a separate concern (CLI flag or env var), not this ticket.
  • Acceptance criteria:
    • Integration/wiring test (pattern-match existing plugins/os-sdlc/tests/runner/post_tool_use_hook_test.rb and gate_wiring_test.rb) asserts EngineFactory.build is invoked (or the hook/CLI path is exercised) with non-Null tracker/notifier instances at both sites — via dependency injection/stub, not a real tea/notify-send call.
    • Existing tests that rely on NullTracker/NullNotifier defaults (e.g. retry_counter_reset_test.rb) continue to pass unmodified — this subtask must not change EngineFactory's default-injection behavior, only what the two call sites explicitly supply.

Subtask 4 — Retire the SkeletonTable .equal? legacy identity branch

Remove StageBriefWriter#legacy?'s table.equal?(SkeletonTable::TABLE) check (plugins/os-sdlc/lib/os_sdlc/runner/stage_brief_writer.rb:22-24) and the Brief.write legacy path it guards, once no live call site still constructs a legacy (non-map) engine — confirm via EngineFactory::Build#legacy_engine (engine_factory.rb) still being reachable; if legacy_engine is still a live path (project has no .sdlc/project.yaml/map), this subtask is blocked until that path itself is retired or given its own non-identity signal.

  • In scope: deleting the identity-branch check and the legacy Brief.write call it guards, iff investigation confirms legacy_engine is unreachable in current production flows (all projects onboarded to maps). If it's still reachable, this subtask becomes "replace .equal? with an explicit legacy: flag threaded through Engine::Context" instead of an outright deletion — the probe did not resolve which of these two shapes applies; that's a judgment call for whoever picks up this subtask, informed by whether SkeletonTable is still referenced by EngineFactory::Build#legacy_engine.
  • Out of scope: removing SkeletonTable itself or Engine::Context's map_loader shape — only the identity-check branch and (if provably dead) its guarded legacy path.
  • Acceptance criteria:
    • If deleted outright: grep -rn "SkeletonTable::TABLE" plugins/os-sdlc/lib returns no .equal? comparison; StageBriefWriter#write always calls @assembler.write; existing StageBriefWriter tests updated/pass without a legacy-path branch.
    • If replaced with an explicit flag: a minitest test constructs Engine::Context with legacy: true and asserts StageBriefWriter#write calls Brief.write, and with legacy: false (or unset) asserts it calls @assembler.write — no .equal?/identity comparison remains in the diff.

Subtask 5 — Resumer#abort! reads the terminal target instead of hardcoding it

Replace the literal "failed" in Resumer#abort! (plugins/os-sdlc/lib/os_sdlc/runner/resumer.rb) with a lookup against the ticket's table. TransitionTable already has terminal_states/terminal?(state) and validates the table at construction time (transition_table.rb), so the fix is either (a) a load-time assertion that every map's terminal_states includes "failed" (raise InvalidTable otherwise, at the same point validate! already runs), keeping abort!'s literal but now backed by a guarantee, or (b) have abort! read the target off the table (e.g. a designated "abort target" or the first/only terminal state) rather than assume the string "failed" at all. Pick (a) unless a map legitimately wants a different abort target than "failed" — the probe found no such map, so (a) is the smaller, lower-risk change; record the choice made.

  • In scope: the load-time check or table-read, plus a test proving an escalated ticket on a map without "failed" in terminal_states fails loudly at map-load time (option a) or that abort! correctly resolves to whatever the table designates (option b) — not both; pick one per the paragraph above.
  • Out of scope: changing what "abort" means functionally (still a terminal, unrecoverable end state) — this subtask only removes the blind hardcode, it does not redesign the abort UX.
  • Acceptance criteria:
    • Minitest test: a TransitionTable built from YAML whose terminal_states omits "failed" raises InvalidTable at load time (if option a), OR Resumer#abort! against such a table still lands the ticket on the table's actual terminal state rather than an unknown/invalid one (if option b).
    • Existing escalation/abort tests (plugins/os-sdlc/tests/runner/escalation_test.rb and any Resumer abort test) continue to pass, and a new test documents the failure mode this closes: "unknown non-terminal state + deactivated escalation = unrecoverable ticket" (per the ticket's round-4 review comment) can no longer happen silently.

Ordering / dependencies

1 and 2 are independent of each other and of 4/5; do them first (each is a clean, independently commit-able adapter). 3 depends on 1 and 2 landing. 4 and 5 are independent of 1/2/3 and of each other — either can land any time, in any order, on their own commits.

Global out-of-scope (whole ticket)

  • Configurable/pluggable tracker or notifier choice per project — both call sites get the real adapters unconditionally (see Subtask 3).
  • GitHub/gh-based tracker adapter — this repo's tracker is Forgejo/tea only (per docs/issue-workflow.md).
  • Cross-platform or third-party push notification services (mobile push, ntfy, Pushover) — no precedent or requirement found; notify-send covers the current dev environment.
  • Any change to gate/sequencing logic in transition_table.rb — none of these subtasks touch routing (ADR-0097).
## Context Deferred from #341 (ADR-0113 park-and-notify): EngineFactory's production call sites (plugins/os-sdlc/bin/os-sdlc-runner, hooks/post_tool_use.rb) still inject NullTracker/NullNotifier, so a parked ticket adds no waiting label, no blocker comment, and no push notification. Build a tea-based tracker port (waiting label + blocker comment) and a push-notification notifier port, and inject them at those call sites. Also retire the table-identity legacy branch (SkeletonTable .equal? checks in Resumer/StageBriefWriter) before phase 3 closes — flagged non-blocking in the #341 review. ## Decomposition (2026-08-12) Evidence gathered via `os-sdlc:code-probe` (ast-grep, read-only) before decomposing: - **Port contract** (confirmed, `plugins/os-sdlc/lib/os_sdlc/runner/null_tracker.rb` / `null_notifier.rb`): `Tracker#add_waiting_label(ticket_id)`, `Tracker#blocker_comment(ticket_id, message)`, `Notifier#push(message)`. - **Injection seam** (`plugins/os-sdlc/lib/os_sdlc/runner/engine_factory.rb`, `Build#tracker`/`Build#notifier`): `@options[:tracker] || NullTracker.new`, `@options[:notifier] || NullNotifier.new`. Both production call sites never pass these options: `hooks/post_tool_use.rb:56` (`EngineFactory.build(root)`, no options at all) and `bin/os-sdlc-runner:15` (`EngineFactory.build(root, **options)` — CLI-parsed `options` has no tracker/notifier flag today). - **Legacy identity branch** (`plugins/os-sdlc/lib/os_sdlc/runner/stage_brief_writer.rb:22-24`): `legacy? => table.equal?(SkeletonTable::TABLE)`, gating `Brief.write` (old flat path) vs `BriefAssembler` (map-based path). - **abort! hardcode** (`plugins/os-sdlc/lib/os_sdlc/runner/resumer.rb`, `abort!`): `Ticket.update_state(@context.db, @ticket[:id], "failed")` — literal string, no table lookup. `TransitionTable` already exposes `terminal_states` / `terminal?(state)` (`transition_table.rb:28,42-43`), validated at load time via existing `validate!`. - No existing push-notification precedent in this repo (checked `plugins/os` and `plugins/os-sdlc` for notify-send/osascript/ntfy/pushover — none found); this is new surface, scoped minimally below. Gate/sequencing logic stays out of scope for every subtask below (ADR-0097): none of these change routing in `transition_table.rb`, they add adapters and fix a data lookup. ### Subtask 1 — Tea-based Tracker adapter Add `OsSdlc::Runner::TeaTracker` (new file, sibling to `null_tracker.rb`) implementing the `Tracker` port: `add_waiting_label(ticket_id)` shells out to `tea issues edit <ticket_id> --add-labels waiting --repo <configured-repo>`; `blocker_comment(ticket_id, message)` shells out to `tea comment <ticket_id> --repo <configured-repo>` (or the equivalent non-hanging path — MEMORY notes `tea comment` hangs on large bodies; keep blocker-comment bodies short/single-line, or write via a temp file the same way this task updates #342, and verify non-interactively before landing). - **In scope:** the adapter class, its shell-out, and the repo-slug source (reuse whatever `plugins/os-sdlc/lib/os_sdlc/issue_source.rb` or `plugins/os-backlog/lib/backlog/issues.rb` already resolves for repo config — do not invent a second config path). - **Out of scope:** GitHub/`gh` support, retry/backoff on tea failures (a failed tracker call should not crash escalation — see Subtask 3 for the injection contract, not this subtask for resiliency policy). - **Acceptance criteria:** - Minitest unit test stubs the shell-out (e.g. injected command runner) and asserts `add_waiting_label` invokes `tea issues edit <id> --add-labels waiting` with the correct repo. - Minitest unit test asserts `blocker_comment` invokes the tea comment path with ticket_id and message, non-interactively (test must not actually hang or shell out to a real network call). - `TeaTracker` is a duck-type match for `NullTracker`: same two public method signatures, verifiable by a shared contract test module run against both. ### Subtask 2 — Push notifier adapter Add `OsSdlc::Runner::PushNotifier` (or similarly named, sibling to `null_notifier.rb`) implementing `push(message)`. Given no existing push mechanism in this repo and the dev machine is Linux/Fedora, scope to a `notify-send`-based desktop notification (matches the local environment; no external service dependency). - **In scope:** the adapter class, shelling out to `notify-send` with the message, guarded so a missing `notify-send` binary degrades to a no-op-with-warning rather than raising (escalation must never crash on a notification failure — matches ADR-0113's "end the turn" framing, which assumes escalation persistence in SQLite succeeds regardless of notification delivery). - **Out of scope:** cross-platform notification backends (macOS `osascript`, mobile push, ntfy/Pushover integration) — no requirement or precedent for those surfaced during the probe; deferred until a concrete need appears. - **Acceptance criteria:** - Minitest unit test stubs the shell-out and asserts `push(message)` invokes `notify-send` with the message text. - Minitest unit test asserts that when the shell-out raises/fails (binary missing, non-zero exit), `push` swallows it without raising and the ticket's escalation persistence (already covered by existing SQLite Escalation tests) is unaffected — i.e. this subtask only needs to prove `push` itself doesn't raise, not re-test Escalation. - `PushNotifier` is a duck-type match for `NullNotifier`: same one public method signature. ### Subtask 3 — Inject real adapters at both production call sites Wire `TeaTracker`/`PushNotifier` into `hooks/post_tool_use.rb:56` and `plugins/os-sdlc/bin/os-sdlc-runner:15`, replacing the implicit `NullTracker`/`NullNotifier` fallback for real runs. `EngineFactory::Build#tracker`/`#notifier` already read `@options[:tracker]`/`@options[:notifier]` — this subtask supplies those options at the two call sites, it does not change `engine_factory.rb`. - **In scope:** constructing `TeaTracker.new`/`PushNotifier.new` (with whatever config each needs, e.g. repo slug) at both call sites and passing them as `EngineFactory.build(root, tracker: ..., notifier: ...)`. - **Out of scope:** making tracker/notifier choice configurable per-project (e.g. via `.sdlc/project.yaml`) — no requirement surfaced for that; both call sites get the real adapters unconditionally. If a future ticket needs a stub mode for local dev, that's a separate concern (CLI flag or env var), not this ticket. - **Acceptance criteria:** - Integration/wiring test (pattern-match existing `plugins/os-sdlc/tests/runner/post_tool_use_hook_test.rb` and `gate_wiring_test.rb`) asserts `EngineFactory.build` is invoked (or the hook/CLI path is exercised) with non-Null tracker/notifier instances at both sites — via dependency injection/stub, not a real tea/notify-send call. - Existing tests that rely on `NullTracker`/`NullNotifier` defaults (e.g. `retry_counter_reset_test.rb`) continue to pass unmodified — this subtask must not change `EngineFactory`'s default-injection behavior, only what the two call sites explicitly supply. ### Subtask 4 — Retire the SkeletonTable `.equal?` legacy identity branch Remove `StageBriefWriter#legacy?`'s `table.equal?(SkeletonTable::TABLE)` check (`plugins/os-sdlc/lib/os_sdlc/runner/stage_brief_writer.rb:22-24`) and the `Brief.write` legacy path it guards, once no live call site still constructs a legacy (non-map) engine — confirm via `EngineFactory::Build#legacy_engine` (`engine_factory.rb`) still being reachable; if `legacy_engine` is still a live path (project has no `.sdlc/project.yaml`/map), this subtask is blocked until that path itself is retired or given its own non-identity signal. - **In scope:** deleting the identity-branch check and the legacy `Brief.write` call it guards, iff investigation confirms `legacy_engine` is unreachable in current production flows (all projects onboarded to maps). If it's still reachable, this subtask becomes "replace `.equal?` with an explicit `legacy:` flag threaded through `Engine::Context`" instead of an outright deletion — the probe did not resolve which of these two shapes applies; that's a judgment call for whoever picks up this subtask, informed by whether `SkeletonTable` is still referenced by `EngineFactory::Build#legacy_engine`. - **Out of scope:** removing `SkeletonTable` itself or `Engine::Context`'s `map_loader` shape — only the identity-check branch and (if provably dead) its guarded legacy path. - **Acceptance criteria:** - If deleted outright: `grep -rn "SkeletonTable::TABLE" plugins/os-sdlc/lib` returns no `.equal?` comparison; `StageBriefWriter#write` always calls `@assembler.write`; existing StageBriefWriter tests updated/pass without a legacy-path branch. - If replaced with an explicit flag: a minitest test constructs `Engine::Context` with `legacy: true` and asserts `StageBriefWriter#write` calls `Brief.write`, and with `legacy: false` (or unset) asserts it calls `@assembler.write` — no `.equal?`/identity comparison remains in the diff. ### Subtask 5 — Resumer#abort! reads the terminal target instead of hardcoding it Replace the literal `"failed"` in `Resumer#abort!` (`plugins/os-sdlc/lib/os_sdlc/runner/resumer.rb`) with a lookup against the ticket's table. `TransitionTable` already has `terminal_states`/`terminal?(state)` and validates the table at construction time (`transition_table.rb`), so the fix is either (a) a load-time assertion that every map's `terminal_states` includes `"failed"` (raise `InvalidTable` otherwise, at the same point `validate!` already runs), keeping `abort!`'s literal but now backed by a guarantee, or (b) have `abort!` read the target off the table (e.g. a designated "abort target" or the first/only terminal state) rather than assume the string `"failed"` at all. Pick (a) unless a map legitimately wants a different abort target than "failed" — the probe found no such map, so (a) is the smaller, lower-risk change; record the choice made. - **In scope:** the load-time check or table-read, plus a test proving an escalated ticket on a map without `"failed"` in `terminal_states` fails loudly at map-load time (option a) or that `abort!` correctly resolves to whatever the table designates (option b) — not both; pick one per the paragraph above. - **Out of scope:** changing what "abort" means functionally (still a terminal, unrecoverable end state) — this subtask only removes the blind hardcode, it does not redesign the abort UX. - **Acceptance criteria:** - Minitest test: a `TransitionTable` built from YAML whose `terminal_states` omits `"failed"` raises `InvalidTable` at load time (if option a), OR `Resumer#abort!` against such a table still lands the ticket on the table's actual terminal state rather than an unknown/invalid one (if option b). - Existing escalation/abort tests (`plugins/os-sdlc/tests/runner/escalation_test.rb` and any `Resumer` abort test) continue to pass, and a new test documents the failure mode this closes: "unknown non-terminal state + deactivated escalation = unrecoverable ticket" (per the ticket's round-4 review comment) can no longer happen silently. ## Ordering / dependencies 1 and 2 are independent of each other and of 4/5; do them first (each is a clean, independently commit-able adapter). 3 depends on 1 and 2 landing. 4 and 5 are independent of 1/2/3 and of each other — either can land any time, in any order, on their own commits. ## Global out-of-scope (whole ticket) - Configurable/pluggable tracker or notifier choice per project — both call sites get the real adapters unconditionally (see Subtask 3). - GitHub/`gh`-based tracker adapter — this repo's tracker is Forgejo/`tea` only (per `docs/issue-workflow.md`). - Cross-platform or third-party push notification services (mobile push, ntfy, Pushover) — no precedent or requirement found; `notify-send` covers the current dev environment. - Any change to gate/sequencing logic in `transition_table.rb` — none of these subtasks touch routing (ADR-0097).
Author
Owner

Additional line item from #341 round-4 review: Resumer#abort! hardcodes terminal 'failed' — add a load-time check that 'failed' is terminal in every map, or have abort! read its target off the table. Failure mode otherwise: unknown non-terminal state + deactivated escalation = unrecoverable ticket.

Additional line item from #341 round-4 review: Resumer#abort! hardcodes terminal 'failed' — add a load-time check that 'failed' is terminal in every map, or have abort! read its target off the table. Failure mode otherwise: unknown non-terminal state + deactivated escalation = unrecoverable ticket.
Author
Owner

Work started on this ticket (subtasks 1-5, in decomposition order: 1,2 then 3, then 4,5).

Work started on this ticket (subtasks 1-5, in decomposition order: 1,2 then 3, then 4,5).
Author
Owner

All 5 subtasks done, one commit each (3f33c63, 3b8964c, 8598f1d, ddec63a, 688528d) + style cleanup ee91639. TeaTracker + PushNotifier adapters TDD'd against the Null ports; both production call sites inject them. Probe findings: legacy_engine was a live silent fallback, so (user-approved) subtask 4 deleted it too — missing project.yaml now raises HarnessError loudly. Subtask 5 = option (a): YamlLoader raises InvalidTable unless a map's terminal list includes "failed". Suite 823 runs green, rubocop clean, opus review approved with no blocking findings. Left open for sign-off.

All 5 subtasks done, one commit each (3f33c63, 3b8964c, 8598f1d, ddec63a, 688528d) + style cleanup ee91639. TeaTracker + PushNotifier adapters TDD'd against the Null ports; both production call sites inject them. Probe findings: legacy_engine was a live silent fallback, so (user-approved) subtask 4 deleted it too — missing project.yaml now raises HarnessError loudly. Subtask 5 = option (a): YamlLoader raises InvalidTable unless a map's terminal list includes "failed". Suite 823 runs green, rubocop clean, opus review approved with no blocking findings. Left open for sign-off.
jared closed this issue 2026-08-13 15:44:40 +00:00
Sign in to join this conversation.
No milestone
No project
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference
jared/cc-os#342
No description provided.