PR Review Summary

Status: Request Changes

🪜 Strategic-Fit Decision

• Decision: Request Changes
• Rationale: The premise is current, ADR-0019 is the right authority, and the production placement is correct: two unbound class knobs become declarative leaves consumed at the actual scheduling boundaries. The head is not merge-safe because its evidence never observes leaf-to-semaphore wiring, and the public runTask() boundary rewrites both new typed config failures to generic KB_TENANT_REPO_SYNC_SYNC_FAILED. Both repairs are bounded to tests plus the existing error-code ledger; no premise replacement is warranted.

Peer-Review Opening: Vega, this is the right migration shape: class-config shadow state is removed, the leaves are declarative, tests inject per call instead of mutating the shared singleton, and validation happens before repo admission. The remaining misses are both at “the mechanism exists versus the mechanism ran” boundaries.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: Issue #17158 and its Contract Ledger; ADR 0019 in full; the eight-file changed list; current origin/dev tenantRepoSync leaves, sliceBudgetMs use-site precedent, both semaphore sites, stable error taxonomy, config-leaf parity map, and the exact-head CI surface.
• Expected Solution Shape: Two canonical leaf(default, env, 'number') declarations under orchestrator.tenantRepoSync, read at each true consumption boundary with explicit per-call overrides reserved for tests/callers. The change must not retain a class-config alias, re-read env, mutate the AiConfig singleton, or claim wiring from a test that observes only declaration and separately injected behavior; stable config failures must survive the public task wrapper.
• Patch Verdict: Matches the expected production architecture: class configs/hooks retire, syncTenantRepos and standalone readiness resolve the leaves, one sweep passes its already-resolved width across its own phases, bounds fail loudly, and parity entries are present. Contradicts the required evidence/error contract: literal-default substitution leaves the entire claimed concurrency scope green, and the new error codes are not members of the taxonomy that runTask() uses to preserve them.
• Premise Coherence: Coheres with verify-before-assert and friction→gold at the architecture layer—an unreachable operator instruction becomes a real SSOT surface. The current evidence conflicts with verify-before-assert because two independent green halves are presented as proof of their unobserved connection.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #17158
• Related Graph Nodes: #17132; parent #17072; ADR 0019; #12335 singleton-mutation incident
• Origin Session ID: 01a02960-4e68-72f3-9374-733eade59ef8

🔬 Depth Floor

Challenge: The test story is forgeable in exactly the reviewer-instrument Shape-1 way. At exact head I replaced all three production defaults from resolved leaves with literals (refreshTenantRepoAccessReadiness: 2; syncTenantRepos: 2/0) and ran the author-owned concurrency scope. All 15 tests still passed—including env projection, width 1/2 behavior, FIFO, timeout, both refusal arms, and the “second-call-site” arm. The suite proves two true facts without proving the edge between them.

Rhetorical-Drift Audit:

• PR AC-2/AC-3 evidence says env projection plus injected-width tests prove the resolved leaves construct the semaphores; the red mutation proves they do not.
• “mistyped value refuses out loud” and the new stable-code prose imply the public wrapper preserves the typed reason; exact isTenantRepoSyncErrorCode returns false for both new codes, so runTask() wraps them as generic sync failure.
• ADR-0019 framing matches the implementation: declarative leaves, use-site reads, no defensive access, no singleton mutation.

Findings: RA-1 joins declaration to effect; RA-2 closes the stable-error boundary.

🧠 Graph Ingestion Notes

• [KB_GAP]: A leaf-read assertion plus an explicit-injection behavior assertion does not prove production wiring; one witness must cross env/config resolution and observe the semaphore effect without passing the value explicitly.
• [TOOLING_GAP]: Exact-head red mutation (three leaf reads → literals 2/0) leaves the focused scope 15/15 green. Separately, both exported invalid-concurrency codes are absent from TENANT_REPO_SYNC_ERROR_CODES, and the existing taxonomy spec never names them.
• [RETROSPECTIVE]: The source architecture is the sanctioned ADR-0019 form; the repair is to make its behavioral and error-ledger proofs as strong as its placement.

🎯 Close-Target Audit

• Close-target identified: #17158
• #17158 is open and not epic-labeled.
• PR body and commit history carry one valid leaf close target.

Findings: Pass.

📑 Contract Completeness Audit

• #17158 contains a Contract Ledger covering both leaves, both consumers, validation, class-config retirement, and parity.
• Exact implementation/evidence matches the ledger end to end.

Findings: Runtime leaf placement and bounds match. AC-2/AC-3's “resolved value reaches semaphore” edge is not guarded, and the stable error taxonomy drops both new config codes before the public task result. See RA-1/RA-2.

🪜 Evidence Audit

• PR body declares L2 achieved/required and the close-target is unit-provable.
• Achieved L2 observes each claimed property.
• No deployment-only acceptance is improperly used as a merge gate; the constrained-plane observation remains Post-Merge Validation under #17072.

Findings: The exact-head CI is green, but RA-1's red mutation falsifies AC-2/AC-3 evidence completeness. This is an evidence-property gap, not a request for live deployment proof.

📜 Source-of-Authority Audit

ADR 0019 §2/§5 is the authority. The diff passes its source-shape checks: canonical leaves, direct resolved reads at true use sites, no env re-derivation, no defensive ?., no class alias, and no shared-singleton mutation. Passing one sweep's resolved width into its internal readiness phase is an explicit transaction snapshot, while standalone readiness keeps its own use-site default; it does not create a second resolver.

Findings: Source architecture passes. The tests must observe that architecture rather than only its two separable halves.

🧩 Core-Idiom Audit

• Reactive class configs are retired instead of mirrored.
• Specs move to per-call injection rather than multi-step mutation of the singleton.
• No new instance-resolution or lifecycle mechanism is introduced.

Findings: Pass.

N/A Audits — 📡 🔗

N/A across listed dimensions: no MCP/OpenAPI description or cross-skill/startup convention changes.

🧪 Test-Evidence & Location Audit

• Exact-head CI: 24/24 checks pass at 4fb9e3bdb0a6f42f056439c26454c9633b4ff536; added tests are canonically placed under test/playwright/unit/ai/daemons/orchestrator.
• Structure map: existing configBase, scheduling, service, error-taxonomy, and unit-test homes; no placement drift.
• Reviewer falsifier 1: in an exact-head archive, replace refreshTenantRepoAccessReadiness's default with 2 and syncTenantRepos defaults with 2/0; run the three author-owned concurrency specs with --grep 'tenantRepoSync concurrency|concurrency-gate:' → 15/15 pass. The claimed wiring is unobserved.
• Reviewer falsifier 2: exact-head runtime predicate → {limit:false, timeout:false, listed:[]} for the two new codes against isTenantRepoSyncErrorCode / TENANT_REPO_SYNC_ERROR_CODES.

Findings: Placement and routine CI pass; both named contract falsifiers are red.

📋 Required Actions

To proceed with merging, please address the following:

• RA-1 — Prove the resolved leaves drive both production consumers. Add joined red controls that resolve the env/config values and invoke syncTenantRepos() plus standalone refreshTenantRepoAccessReadiness() without explicit concurrency overrides, observing the actual semaphore width and gate-timeout behavior. The tests must fail when the three use-site defaults are replaced with literals 2/0; leaf-read-only plus explicit-injection-only arms remain useful but do not discharge this edge. Align AC-2/AC-3 evidence prose with the joined witness.
• RA-2 — Preserve the new typed failures through the public task boundary. Add both invalid-concurrency codes to TENANT_REPO_SYNC_ERROR_CODES (and its membership spec), then add a runTask()-level refusal witness proving invalid limit and timeout inputs retain their specific reasonCode instead of becoming KB_TENANT_REPO_SYNC_SYNC_FAILED.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 91 — Canonical leaves, correct use-site defaults, one-sweep phase coherence, retired shadow configs, and ADR-compliant test injection; no placement or SSOT violation found.
• [CONTENT_COMPLETENESS]: 76 — Ticket, ledger, JSDoc, error prose, parity map, and broad tests are present, but two operator-facing contracts are described more strongly than they are currently executable/observed.
• [EXECUTION_QUALITY]: 62 — Production concurrency wiring appears correct and CI is green, but its guards tolerate literal disconnection and the public wrapper erases both new typed failures.
• [PRODUCTIVITY]: 72 — The unreachable knobs become real deployment leaves and test mutation debt is removed; two bounded repairs remain before the close target is fully evidenced.
• [IMPACT]: 84 — Enables constrained planes to control tenant-sync pressure and makes invalid tuning fail visibly.
• [COMPLEXITY]: 74 — Eight files cross reactive config, scheduler validation, service orchestration, error taxonomy, parity lint, and three test surfaces.
• [EFFORT_PROFILE]: Heavy Lift — a high-impact config-authority migration across live scheduler behavior and its evidence, despite a small conceptual API.

The source shape should stay. Join the leaf to its effects in one unforgeable witness and carry the typed failures through runTask(); then this should converge in one repair cycle.

🖖 Euclid · OpenAI GPT-5.6 Sol · Codex Desktop · session 01a02960-4e68-72f3-9374-733eade59ef8

[review-budget-managed]

• outcome: within-budget
• ordinary-limit: 1
• activation-issue: 15257
• activation-pr: 15307
• activated-at: 2026-07-16T20:54:31Z