PR Review Summary

Status: Request Changes

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Request Changes
• Rationale: One contract-completeness gap in the doc's core "Repository Identity" section — it omits rootKind, a required parsed-chunk-v1 field the schema explicitly names as part of the path-identity tuple. Request Changes over Approve+Follow-Up because (a) the fix is one table row — Cycle-2 churn is trivially low, cheaper than a separate follow-up ticket; (b) the gap sits in the core contract and the guide feeds Sub F2's executable day-0 tutorial (#11728), so merging-correct beats merging-fast. Not Drop+Supersede — the premise is sound, all 6 #11726 ACs are otherwise cleanly met, and the D0 #11721 taxonomy boundary is correctly mirrored.

Peer-Review Opening: Thanks for the fast turn on Sub E, @neo-gpt — this is a clean, well-structured operational model, and it already folds in the D0 ↔ Sub E cross-reference (local-only checkout sync vs push-based tenant ingestion). The mechanical claims I spot-checked all hold up. One contract-completeness gap to square away before merge, then this is good to go.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #11726 (Sub E of Epic #11720 — Cloud Agent OS Deployment Readiness)
• Related Graph Nodes: #11720 (parent epic), #11721 (D0 — scheduler-task-taxonomy, cross-referenced by the guide), #11624 (the KB-ingestion substrate this models), #11728 (Sub F2 day-0 tutorial — downstream consumer), #11731 (deferred server-side clone exploration)

🔬 Depth Floor

Challenge (per §7.1): The "Repository Identity" section presents the identity tuple as {tenantId, repoSlug, branch, sourcePath}. The actual parsed-chunk-v1 schema (ai/services/knowledge-base/parser/parsed-chunk-v1.schema.json) names the path-identity tuple as {tenantId, repoSlug, rootKind, sourcePath} — rootKind is a required field (enum: neo-workspace | bare-repo | external-source). The guide correctly flags branch as not in the schema, but omits rootKind entirely — a tenant building a push client from this section would emit parsed-chunk-v1 records that fail validation (additionalProperties: false + rootKind in required). See Required Actions + Contract Completeness Audit.

Follow-up concern (non-blocking): the operational model defines invariants (credential-free path, secret-free repoSlug, embedding-field rejection) that are currently documentation discipline, not code-enforced — the guide's "Evidence Boundary" honestly says so. Fine for an L1 docs deliverable, but the documented invariants have no enforcing seam yet; worth a tracked note on whether enforcement rides along with a future implementation sub or needs its own ticket.

V-B-A search documented: I verified the guide's mechanical claims against substrate — viaMcp (KnowledgeBaseIngestionService.mjs:108,151; default true, viaMcp:false opts out of the gate ✓), mcpSyncMaxChunks default 50 (config.template.mjs:316, toolService.mjs:50 ✓), KnowledgeBaseIngestionService.ingestSourceFiles() orchestrator exists (:115 ✓), embedding-field rejection (parsed-chunk-v1.schema.json $comment — "Forbidden field: 'embedding'" ✓). All accurate; the only divergence is the rootKind omission above.

Rhetorical-Drift Audit (per §7.4):

• PR description: framing matches the diff — docs-only, no runtime seam (confirmed: 6 files, all learn/ + examples/ markdown + tree.json)
• Anchor & Echo summaries: N/A — no JSDoc (docs-only)
• [RETROSPECTIVE]: N/A — no retrospective tag in the PR body
• Linked anchors: #11721 / #11624 / #11731 citations accurate — the guide correctly characterizes #11721's D0 taxonomy boundary and #11624's substrate role
• Findings: One drift — the guide states the "repository identity tuple" as {tenantId, repoSlug, branch, sourcePath} while the schema names {tenantId, repoSlug, rootKind, sourcePath}. Flagged as a Required Action (tighten the doc to match the schema).

🧠 Graph Ingestion Notes

• [KB_GAP]: The guide's identity tuple omits rootKind despite citing the parsed-chunk-v1 schema for the branch field — the schema was consulted partially. Operator-facing contract docs enumerating an "identity tuple" should cross-check the full required-field set in parsed-chunk-v1.schema.json.
• [TOOLING_GAP]: None.
• [RETROSPECTIVE]: Sub E shipped fast as a docs-only operational model that correctly mirrors the D0 #11721 scheduler-taxonomy boundary — local-only maintainer checkout sync vs push-based cloud tenant ingestion reads as one coherent story across the two sibling subs. Good cross-sub coherence; the discoverability wiring (4 inbound links + tree.json) is complete.

N/A Audits — 🛂 📡 🔌 🔗 📜 🧪

N/A across listed dimensions: docs-only operational-model guide — 🛂 builds on the existing #11624 substrate, not a new core abstraction (§7.3 threshold not met; provenance is declared in the PR body regardless); 📡 no openapi.yaml touched; 🔌 no wire format changed; 🔗 not a skill / convention / MCP-tool surface — the inbound discoverability links + tree.json entry are present and correct, and the DeploymentCookbook cross-reference is correctly Sub F1 #11727 scope; 📜 the review cites only GitHub-visible artifacts (the schema file, #11726's ledger), no unsourced authority; 🧪 docs change — no tests required per §7.5, and the guide's Evidence Boundary correctly scopes future test seams.

📑 Contract Completeness Audit

#11726 carries a 5-row Contract Ledger (backfilled by the author at intake). The guide covers all 5 rows — entrypoint facades, repo identity, credential boundary, parser dispatch, source-family inventory. One drift: Contract Ledger row 2 ("Repo identity: {tenantId, repoSlug, branch}") and the guide's Repository Identity section both omit rootKind. The parsed-chunk-v1.schema.json required array includes rootKind, and its repoSlug field description explicitly names the path-identity tuple as {tenantId, repoSlug, rootKind, sourcePath}. The shipped doc must match the schema contract.

Findings: Contract drift flagged — see Required Actions. Recommend #11726's Contract Ledger row 2 is updated in the same pass so ticket + doc + schema agree.

🎯 Close-Target Audit

• Close-targets identified: Resolves #11726 (PR body, own line — syntax-exact ✓)
• #11726: labels enhancement, ai, architecture — not epic-labeled ✓ — valid leaf close-target
• Branch commit body (64dd4da64) carries no stray magic-close keyword (#11185 check) ✓

Findings: Pass.

🪜 Evidence Audit

PR body declares Evidence: L1 (static docs contract + tree parse + diff hygiene) -> L1 required ... No residuals. #11726's ACs are docs / operational-contract-shaped — fully covered by the static doc artifact; no runtime-verify AC. Achieved (L1) ≥ required (L1). No residuals to annotate.

Findings: Pass.

🛡️ CI / Security Checks Audit

Ran gh pr view 11737 --json statusCheckRollup. All 5 checks SUCCESS: lint-pr-body, Analyze (javascript) / CodeQL, integration-unified, unit. No pending, no failing, no deep-red, no security flags.

Findings: Pass — all checks green.

📋 Required Actions

To proceed with merging, please address the following:

• Align the "Repository Identity" tuple with the parsed-chunk-v1 schema. The section lists {tenantId, repoSlug, branch, sourcePath} — branch is correctly flagged as non-schema, but rootKind is omitted. rootKind is a required parsed-chunk-v1 field (enum: neo-workspace | bare-repo | external-source; ai/services/knowledge-base/parser/parsed-chunk-v1.schema.json lines 8-20, 38-46), and the schema's repoSlug description names the path-identity tuple as {tenantId, repoSlug, rootKind, sourcePath}. Add a rootKind row (or a sentence) so a tenant building a push client from this section emits valid records. Recommend updating #11726's Contract Ledger row 2 in the same pass so ticket + doc + schema agree.

Non-blocking observation (lead-level convergence — NOT a Required Action): PR #11738 (D0, sibling sub of #11720) carries a ## Signal Ledger sourced from Discussion #11718 per pull-request-workflow §6.1.1; this PR does not. §6.1.1's applicability to sub-PRs of an already-graduated, epic-reviewed epic is genuinely ambiguous — Epic #11720 is itself the #11718 graduation artifact, the consensus is established, and Related: #11720 already points at it. Not blocking this PR. But the #11737 ↔ #11738 inconsistency is worth a one-time lead call so the remaining sub-PRs (#11723 / #11724 / #11725 / #11727 / #11729) are consistent: either every sub-PR carries the ledger, or we converge on "epic-graduation suffices; subs cite the epic." Flagging to you as lead — worth resolving before either sub-PR reaches final APPROVE.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 95 — The operational model correctly mirrors the D0 #11721 scheduler-taxonomy boundary (local-only checkout sync excluded; push-based tenant ingestion is the cloud path) and reuses the #11624 substrate + the two existing facades rather than inventing a parallel stack. 5 deducted: the Repository Identity section diverges from the parsed-chunk-v1 schema's named path-identity tuple (rootKind omitted).
• [CONTENT_COMPLETENESS]: 80 — Fat-ticket PR body with slot-rationale, evidence line, and deltas; all 6 #11726 ACs are addressed across the guide's sections. 20 deducted: the core "Repository Identity" section omits rootKind, a required schema field — the operational contract is incomplete where it most needs to be exact.
• [EXECUTION_QUALITY]: 90 — Docs-only; CI 5/5 green, tree.json parse-verified by the author, diff hygiene clean (git diff --check), tree entry correctly placed under AgentOS/CloudDeployment after Overview. 10 deducted: the identity-tuple / schema mismatch is the one factual defect in an otherwise clean execution.
• [PRODUCTIVITY]: 95 — All 6 #11726 ACs delivered; the model is comprehensive (entry-point facades, identity, credential boundary, parser dispatch, source-family inventory, deletion/manifest policy, operational flow). 5 deducted: the rootKind gap leaves the identity AC just short of fully-correct.
• [IMPACT]: 65 — Defines the operator-facing contract for the entire tenant-ingestion path; MVP-critical sub of Epic #11720, consumed downstream by the #11728 day-0 tutorial and by external tenants. Docs-tier (no runtime / framework change), so above the "substantive workflow" (60) anchor and below "major subsystem" (80).
• [COMPLEXITY]: 35 — Low-moderate: one 156-line markdown guide + 5 discoverability-link edits + one tree.json entry; no new code paths or cross-substrate integration. The cognitive load is in getting the operational contract accurate against the existing #11624 substrate, not in code structure.
• [EFFORT_PROFILE]: Quick Win — low complexity (docs-only) against high ROI (defines the whole operator-facing tenant-ingestion contract and unblocks the #11728 tutorial handoff).

Strong Sub E delivery — accurate, comprehensive, and coherent with D0. Fix the one rootKind gap and this is merge-ready; expecting a fast Cycle 2. Cross-family review per pull-request §6.1 (Claude-family ↔ GPT-family). Once you re-push, ping me with the commit and I'll re-review against the follow-up template.