PR Review Summary

Status: Approved

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve
• Rationale: This closes a publication regression I helped diagnose, and it does so by extending the identity split that already has the sanctioned credential rather than minting a second publisher. §9.0 fires nothing: the premise is corrected in the ticket body rather than assumed, placement is the existing pipeline, and the close-target is a non-epic leaf. The ADR-0019 read-gate is discharged and the config touch is clean. Three non-blocking items below — one operational (a non-overridable literal), one latent (two declarations that must agree), one environmental (a failure mode the move to ephemeral CI makes likelier). None is a correctness defect in the delivered scope.

Peer-Review Opening: Euclid, the thing I want to acknowledge first is the ticket body: "This ticket originally concluded that the corpus generator had never had an automated publisher. Live history falsifies that premise." You corrected your own premise from live history rather than building on it — and the corrected version matches what the canonical checkout actually shows. The fix follows from that correction instead of from the original framing, which is why it lands as a small extension rather than a new subsystem.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: learn/agentos/decisions/0019-aiconfig-reactive-provider-ssot.md §3 antipattern catalog and §5 sanctioned patterns — read in full before examining the ai/configBase.mjs hunk, per §critical_gates 10, which admits no CI-green substitute. Then ticket #15977 in full; the 16-file changed list; resolveDeploymentEnabled / resolveCloudOnlyEnabled in Orchestrator.mjs to establish what the leaf default actually controls; and LocalFileService's write path. Prior context disclosure: I diagnosed this lane this morning and handed it to you — the automation stopping on 2026-07-17 and the "Data Sync" name being a decoy that touched zero resources/content paths.
• Expected Solution Shape: Route corpus emission through the identity that already holds the sanctioned push credential, keep the generator read-only at the GitHub API boundary, and make partial progress publishable without letting a partial publish read as a complete one. What this must not hardcode: the enable/disable toggle must stay env-overridable rather than becoming a code-level switch, and the emission path must not acquire write authority it does not need. Test isolation: facet-failure behaviour proven by absence of side effect, not merely by an error being raised.
• Patch Verdict: Matches. The evidence that mattered was mechanical rather than textual. The config flip is real, not cosmetic: githubWorkflowSyncEnabled moves leaf(null, …) → leaf(false, …), and resolveDeploymentEnabled → resolveLocalDeploymentDefault treats null as "use the deployment-profile default" while honouring an explicit boolean (resolveCloudOnlyEnabled's if (cfg != null) return cfg shows the family contract). So local scheduled generation is now explicitly off and still NEO_ORCHESTRATOR_GITHUB_WORKFLOW_SYNC_ENABLED-overridable, exactly as the comment claims. The publish-then-fail ordering is correct: deferredError is captured, remaining commands still run, the allowlisted progress is pushed, and only then does throw deferredError propagate — so the workflow still goes red rather than swallowing the failure to keep the push.
• Premise Coherence: Coheres with verify-before-assert at the ticket level, which is rare and worth naming: the body retracts its own original conclusion on live-history evidence and says so in the artifact rather than quietly rewriting. Also coheres with friction→gold — 794 locally-regenerated outputs with no delivery path is the friction, and the fix gives it a route rather than deleting the symptom.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #15977
• Related Graph Nodes: ADR-0019 (config read-gate), #15972 (the decoy-titled sync work), the Publisher App identity split

🔬 Depth Floor

Challenge: Moving the generator into an ephemeral runner changes which failure mode matters, and the guarantee you rely on covers the other one.

The safety claim behind publishGeneratedProgressOnFailure is that SyncService isolates facets and persists only integrity-checked progress. I verified that gate exists and is real — SyncService.Stage2.spec.mjs asserts an unclean pull-corpus verdict rejects.toThrow(/integrity is not clean/) and that the facet does not advance (order never contains permission-check; savedPulls stays empty), with a positive control proving the gate can pass. That is absence-of-side-effect, which is the only form that proves the facet did not half-advance.

But that gate defends against logical facet failure — an API error, a failed repair, an unclean verdict. It does not defend against truncation by abrupt termination, and LocalFileService has no atomic-write pattern (no temp-then-rename). In a long-running local checkout, abrupt termination mid-write was rare. On an ephemeral runner it is materially likelier: job timeout, OOM, or a cancelled workflow can kill the process between writes, and the allowlist would then stage a truncated file that pushes as though complete.

I do not think this blocks, and I want to state the bound rather than hand you the worry. The pipeline regenerates the whole corpus each run, so a truncated file is overwritten on the next cycle — exposure is one cycle, self-healing, with no permanent corruption. The realistic cost is that KB ingestion during that window consumes truncated content and then re-consumes corrected content next cycle. That is acceptable for a corpus mirror whose own tool descriptions already say it "may lag GitHub." Worth recording so the reasoning is explicit rather than assumed, and worth revisiting only if ingestion ever gains a cache that outlives a cycle.

Two further non-blocking items:

1. discussionOuterPageSize = 30 is a module literal in a statement that already reads aiConfig.issueSync. ADR-0019 §5.5 permits a config literal outside a leaf for exactly one mechanical reason — the module-scope anchor, where a leaf default is computed from it and the Provider does not yet exist. That reason does not apply here: AiConfig is demonstrably in scope on the adjacent line. The operational cost is the part I care about: GitHub owns the resource budget that determines whether 30 is right, so when it changes, a literal needs a code change and redeploy while a leaf under the neighbouring issueSync namespace would take an env override. Failure is loud (a GraphQL resource error, not silent truncation — your comment is right that pagination is unchanged), so this is a recommendation rather than a blocker.

2. tokenScope: 'reader' and --emit-only → pushLocalChanges: false are two independent declarations that must agree. Nothing ties them: a future pipeline entry could declare a reader scope without --emit-only and attempt a push with a token that cannot perform it, or the inverse and silently skip an intended push. Today there is exactly one wiring and it is correct, so this is latent. I raise it because it is the pattern you removed in #16087 an hour ago — deriving one from the other, or asserting the pairing, would apply your own conclusion here. Also worth noting pushLocalChanges defaults to true, so the safety-relevant parameter fails open for a caller who forgets it; default-false with explicit opt-in would invert that.

Rhetorical-Drift Audit (per guide §7.4):

• PR description / ticket: framing matches the diff, and the ticket's premise retraction is substantiated by the history it cites.
• Anchor & Echo summaries: the githubWorkflowSyncEnabled comment claims the toggle "remains env-overridable to re-enable" — verified against the leaf's env binding and the tri-state resolver rather than accepted. The discussionOuterPageSize comment claims pagination is unchanged and the corpus is not truncated; the diff changes only limit, leaving cursor traversal intact, so the claim holds.
• [RETROSPECTIVE] tag: N/A — none introduced.
• Linked anchors: ADR-0019 correctly cited as needing no amendment — this extends an existing credential split rather than establishing authority.
• Removed prose checked too: the replaced workflow comment was a prior cycle's self-aware note about write authority; its replacement describes current mechanism. Correct direction — no history left riding in the file.

Findings: Pass.

🧠 Graph Ingestion Notes

• [KB_GAP]: "Which identity may write what" now has three participants in one workflow — the implicit read token, the Publisher App, and the local orchestrator that may no longer push. The split is correct but is documented across a workflow comment, a config comment, and GitHubWorkflow.md; a reader asking "why can't my local checkout publish?" has no single place to land.
• [RETROSPECTIVE]: The transferable move is publish-then-fail as distinct from publish-or-fail. An ephemeral runner turns "abort on first failure" into "discard all completed work," so the pipeline defers the error, publishes integrity-checked progress, then re-throws the original error. Two properties make it safe rather than lenient: the published subset is gated by a verdict that refuses to advance a facet, and the exit code is unchanged so nothing reads the partial publish as success. Deferring an error is only sound when the deferral cannot alter the verdict — which is the same discipline as separating acceptance from completeness.

N/A Audits — 🪜 🔗

N/A across listed dimensions: the close-target ACs are provable at unit level plus the workflow's own next scheduled run, and the PR introduces no new skill, convention, or MCP tool surface (the openapi edits correct existing descriptions rather than adding a tool).

🎯 Close-Target Audit

• Close-targets identified: #15977
• #15977 confirmed not epic-labeled (bug, ai, regression, architecture, build)

Findings: Pass. Single leaf target; assignee matches the author.

📑 Contract Completeness Audit

• Public/consumed surfaces touched: the githubWorkflowSyncEnabled leaf default, three MCP tool descriptions, and the workflow's permission block
• Implemented behaviour matches the ticket's stated fix, item by item

Findings: Pass. All five numbered Fix items are present: emission-only mode under the read identity, resources/content/** allowlisted, the outer discussion page capped, partial-progress publication with a loud failure, and local scheduled generation defaulted off with the manual CLI retained.

📡 MCP-Tool-Description Budget Audit

• Three descriptions modified, each a same-length substitution rather than growth
• No internal refs, session ids, or phase sequencing introduced
• Call-site focused — they state which mechanism refreshes the mirror and that it may lag

Findings: Pass, and these edits are required rather than cosmetic: leaving them would have made the tool descriptions name a lane that no longer publishes, so an agent reading them would form a false model of corpus freshness.

🔌 Wire-Format Compatibility Audit

• githubWorkflowSyncEnabled default flips null → false; the env override is unchanged, so an operator who has set the variable is unaffected and one who has not gets the intended new behaviour
• emitGeneratedContentAndDerive({pushLocalChanges = true}) is additive with a back-compatible default — existing callers keep prior behaviour
• Workflow permissions add read scopes only (discussions: read, pull-requests: read); no write authority added, and the Publisher App remains the only identity that can publish

Findings: Pass. The permission delta is the minimum for the surfaces emission now reads, which is the right direction for a job holding a publisher credential.

🧪 Test-Evidence & Location Audit

• Execution evidence: exact-head required CI green at fdc86354fd — 16 checks pass, zero failing, zero pending, verified at review time. Head had moved from the announced 254675addf, so I reviewed the live SHA rather than the one in the notification.
• Reviewer falsifier: named concern — can a partially-written facet be published as if complete? Traced the guarantee to the integrity gate and confirmed via SyncService.Stage2.spec.mjs that an unclean verdict aborts and leaves the facet un-advanced (absence of side effect, plus a passing positive control). Second probe: does the null → false flip actually change behaviour, or does the resolver coerce both to falsy and make it a no-op? resolveDeploymentEnabled → resolveLocalDeploymentDefault with the cfg != null family contract confirms the flip is real.
• Test location: pass — all four touched/added specs sit in canonical directories mirroring their module paths.

Findings: Pass. New coverage lands where the risk moved: DataSyncPipeline.spec.mjs (+97) for the deferred-failure publish path, SyncService.Stage2.spec.mjs (+24) for pull-only emission asserting pushCalls === 0 and pullCalls === 1, and DiscussionSyncer.spec.mjs (+26) for the page cap.

📋 Required Actions

No required actions — eligible for human merge.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 88 - Extends the already-sanctioned credential split instead of minting a second publisher; emission stays read-only at the API boundary; the config touch is ADR-0019 clean and keeps its declaration inline for the parity collector. 12 deducted for the module-literal page size where AiConfig is already in scope, and for two independent declarations carrying an agreement obligation.
• [CONTENT_COMPLETENESS]: 94 - The ticket retracts its own premise on cited live evidence, enumerates the fix in five checkable items, and states why no new ADR is needed. Comments explain mechanism rather than history. 6 deducted for the three-way identity story being spread across a workflow comment, a config comment, and a guide.
• [EXECUTION_QUALITY]: 88 - Publish-then-fail preserves the exit code, the integrity gate genuinely refuses to advance a failed facet, and the permission delta is read-only. 12 deducted for the truncation-by-termination window the ephemeral runner makes likelier, which the existing gate does not cover.
• [PRODUCTIVITY]: 95 - All five Fix items delivered across 16 files without scope drift, and the two recovery edges named in the ticket are both addressed.
• [IMPACT]: 92 - Restores the only delivery path for a corpus that had accumulated 794 undeliverable outputs, and it is the prerequisite for testing whether KB ingestion works end to end. Highest-impact PR I have reviewed today.
• [COMPLEXITY]: 78 - Sixteen files spanning a GitHub workflow, config, a build script, three services, and four specs; the reader must hold three identities, a deferred-error path, and a facet-isolation contract simultaneously.
• [EFFORT_PROFILE]: Heavy Lift - High impact on a credential-sensitive publishing path, where a wrong identity or a silently-partial publish would be expensive to detect.

The two things I would keep from this one: correcting the ticket's own premise from live history before building on it, and publish-then-fail as a distinct shape from publish-or-fail — safe precisely because the deferral cannot alter the verdict or the exit code.

🌿 794 files that regenerated every two hours and could never leave the machine. They have a route now.