# Architect Review — Upstream dual-coach port (pass 1)

Reviewed exactly the immutable Planner artifact:
`/home/cube/projects/richard/traning coach/.gjc/_session-019fadee-1400-7000-8924-4c6371ae6282/plans/ralplan/019fadee-1400-7000-8924-4c6371ae6282/stage-01-planner.md`

Planner SHA-256: `6497e636847f336bb0c135f3153d32296d692dd1d708b5bf97ca2c5a35e06118`

## Summary
The plan is architecture-safe and execution-ready. It treats the three stale commits as behavioral evidence, maps them onto the current Telegram plugin and scheduler owners, protects the dirty legacy worktree and private Profile boundary, requires mergeability and current-upstream regression proof, and stops the candidate before every live/provider/customer capability.

The recommended fresh branch and replacement PR is materially safer than replaying or rewriting the 6,916-commit-behind branch. Acceptance, proof artifacts, rollback preservation, conflict escalation, and offline stop points are concrete enough to proceed.

## Claims
- The Planner correctly identifies architectural rather than textual drift: `4305027f6` adds 35,627 lines and targets the legacy `gateway/platforms/telegram.py`, while base `41a07f5b8` owns Telegram in `plugins/platforms/telegram/adapter.py` and current group-gating tests import that plugin (Planner lines 4–6, 61–76).
- Repository inspection corroborates the commit chain: `4305027f6` is directly based on `b88d0007c`, followed by `d9e03e938` and `0dc0f228d`; the legacy Hermes worktree is on `feature/dual-coach-lifecycle` and contains unrelated modified and untracked BlueBubbles, Photon, CLI, tools, test, `.gjc`, and MacBook files.
- Base `41a07f5b8` contains the 1,593-line bundled Telegram adapter with `register(ctx)` plugin ownership, the current group-gating suite imports `plugins.platforms.telegram.adapter`, and `pyproject.toml` packages both `cron` and `plugins`. The Planner's current-owner mapping is therefore file-backed.
- The private Profile checkout contains the pinned typed surfaces named by the plan, including `CustomerActionContinuity`, `build_registered_daily_customer_projection`, `CustomerWeeklyReviewSource`, and `build_customer_weekly_review_source`; the public port can be validated without copying private implementations.
- The local canonical pointer independently confirms that Topic 59 review identity and canonical-owner identity are distinct, unknown and audit-pending outcomes are no-resend, preflight is read-only, and live Gate-D remains human-only (`PILOT_RUNBOOK.md`).

## Analysis
### Spec compliance
The plan solves the requested port rather than substituting a cherry-pick exercise. It freezes the requested public base and source/Profile pins, inventories source invariants before implementation, classifies every broad foundational addition as required/replaceable/omitted, and makes source-to-current API ownership a prerequisite (Planner lines 79–83). The acceptance criteria require a branch rooted exactly at `41a07f5b8`, a mergeable replacement PR, plugin/scheduler integration, strict typed Profile compatibility, exact identity isolation, append-only revisions, immutable approval/body pins, at-most-once delivery, and an offline all-disabled candidate (lines 145–154).

The public/private boundary is explicit and fail closed. Private Profile source, data, identifiers, credentials, raw-store access, and executable Gate-D material are excluded; the required API matrix records only signatures and failure contracts, tests use sanitized fixtures, and pinned private compatibility is reserved for private CI (lines 51–77, 124–137, 172–176). Missing or malformed APIs cannot silently degrade to JSON, permissive reflection, alternate runtime selection, or raw storage.

### Architecture and conflict ownership
The strongest antithesis is that preserving the original commits would retain provenance and reduce reimplementation risk. Here that benefit is outweighed by the 35K-line foundational drop, the legacy-to-plugin migration, and conflicts in adapter, scheduler, and gating seams. The plan answers this correctly: current owners win, safety invariants win over source hunks, scratch cherry-picks are discovery-only, and obsolete aliases/duplicate handlers are prohibited (lines 88–109).

Conflict ownership is sufficiently gated. Every semantic mismatch must identify its invariant and current owner before resolution; changed plugin/Profile signatures, state transitions, storage schemas, provider boundaries, callback authorization, and scheduler recovery paths require a named reviewer/architect decision. Missing safe seams, incompatible Profile APIs, raw-storage requirements, or unrepresentable identity/body pins stop the port instead of inviting executor guesswork (lines 101–109, 164–169). The executor should populate the named owner in the decision table before resolving each such conflict; that is an execution artifact required by the existing plan, not additional scope.

The integration shape preserves existing Telegram plugin registration and generic semantics rather than adding a parallel gateway adapter. The plan also delays scheduler/delivery work until identity, authority, approval, and exact-body flow are established, which prevents a UI or scheduler route from becoming an accidental bypass (lines 81–84).

### Branch replacement and rollback
The sibling detached worktree strategy is concrete and non-destructive: create from the verified object `41a07f5b8`, require an empty status before branch creation, never run stash/reset/clean/checkout/tests in the legacy worktree, and restart mapping rather than silently rebasing if policy later requires a newer upstream tip (lines 79, 88–99). The observed dirty legacy worktree makes this isolation load-bearing.

Option A retains the old PR, old remote history, and dirty local worktree while producing an independently reviewable head. The plan records old and new SHAs, avoids force-pushing #74072, requires CI/mergeability/review evidence, and closes the old PR only as a supersession action after replacement acceptance (lines 30–46, 85, 143, 146, 159–162, 178). Closing a PR does not replace or mutate the preserved source commits/worktree; the ancestry record and immutable source SHAs remain the rollback/forensics anchor. Option B is correctly conditional on a green immutable candidate, explicit owner choice, and an exact force-with-lease SHA, and is not the default.

### Test fidelity
The test plan covers observable safety behavior rather than mocks alone: exact positive and negative triples, generic-path isolation, immutable child reapproval, byte/digest changes including Unicode and whitespace, pre-invocation versus ambiguous outcomes, duplicate/restart/reconciliation call counts, disabled scheduler behavior, and malformed private APIs (lines 112–123). Integration tests exercise current `TelegramAdapter` ingress/callback ordering, distinct customer/trainer/reviewer identities, isolation across customers, delivery revoke races, scheduler recovery, and a recording transport. Private Profile fidelity is anchored by the exact signature matrix and pinned private compatibility CI, while package portability is separately tested with a synthetic Profile fixture.

Excluding the known contaminated combined fixture from release proof is appropriate because the plan does not suppress or waive it: it remains diagnostic, while hermetic focused suites and relevant unaffected plugin/scheduler regressions are required. CI summaries, raw private logs, changed-file allowlists, provider-call counts, secret/privacy scans, and manual review make the acceptance evidence auditable (lines 124–143, 156–162).

### Deployment stop points
The candidate boundary is explicit and enforceable. It is built from the reviewed replacement SHA with every delivery/activation flag false; validation uses only local fake adapters and read-only preflight; provider counts must remain zero except for explicitly fake simulated delivery tests. The manifest must say `manual Gate-D pending / rollout approval pending`, and release escalation stops on credentials, accounts, live Telegram, customer activation, provider calls, Gate-D, delivery, or rollout requests (lines 86, 132–143, 153, 162, 164–169). This matches the canonical pointer's human-only boundary.

## Root Cause
The original branch is not merely behind; it encodes dual-coach behavior against owners and APIs that changed across 6,916 upstream commits, including migration of Telegram into a bundled plugin. Blind history replay would conflate behavioral intent with obsolete architecture and could also capture unrelated dirty-worktree state. The plan fixes that root cause by rebuilding the contract at current ownership seams under pinned identity, authority, immutability, and delivery invariants.

## Findings
No reportable CRITICAL, HIGH, MEDIUM, or LOW issues. The plan's conflict gates, rollback strategy, acceptance proof, and human-only boundary are concrete; implementation evidence remains mandatory before any code or release approval.

## Recommendations
1. Execute Option A exactly: isolated sibling worktree, fresh branch from `41a07f5b8`, small invariant-owned commits, and replacement PR without rewriting #74072.
2. Require the conflict/API decision table and named owner sign-off before merging any adapter/Profile signature, persistence, authorization, scheduler, or provider-boundary change.
3. Treat the Profile `57aaf75` private compatibility run, current Telegram/plugin/scheduler regressions, mergeable CI head, privacy scan, and offline candidate manifest as hard acceptance evidence, not optional follow-up.
4. Preserve the old PR/source head and dirty worktree through replacement acceptance; do not delete or force-update them as part of supersession.
5. Stop at the offline candidate manifest. Gate-D, secrets, accounts, customer activation, provider delivery, and rollout approval remain human-only.

## Architectural Status
`CLEAR`

## Code Review Recommendation
`APPROVE`

## Tradeoffs
| Option | Reviewability / drift safety | Rollback safety | Decision |
|---|---|---|---|
| Fresh branch and replacement PR | Highest; current APIs and small commits are explicit | Highest; old PR/head/worktree remain intact | Adopt |
| Green branch then force-with-lease old PR | Preserves discussion but rewrites review history | Conditional on owner approval and exact lease | Do not use by default |
| Cherry-pick three stale commits | Lowest; conflict resolution can preserve obsolete owners and 35K-line baggage | Scratch-only diagnostic value | Reject as implementation path |