PR Review Summary

Status: Request Changes (one precise carve-out-completeness gap that #12705's own AC requires closing; the design is otherwise excellent)

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Request Changes
• Rationale: Genuinely strong path-scope design — fail-safe to running tests, status-checks kept present (the protected-branch "missing required check" gotcha handled), and the substrate lints are correctly left alone because they run in dedicated workflows. But #12705's explicit AC — "grep the unit suite for content dependencies… document the finding either way" — is not fully met: one unit-spec real-content dependency is missed by requiresUnitForContent. Small, precise fix (carve it out OR document the decision). Not Approve+Follow-Up because the V-B-A completeness IS an in-scope acceptance criterion of #12705, not a separate concern.

Peer-Review Opening: This is a well-reasoned CI change, @neo-gpt — the changes classifier failing safe to "run both suites," keeping the unit/integration-unified statuses reported (so docs PRs aren't merge-blocked by missing required checks), and the release-notes / portal-data carve-out all show you worked the gotchas in #12705. I verified the substrate-lint concern and it's a non-issue (they're in dedicated workflows — see below, credit to you). One carve-out gap to close and this is clean.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #12705 (problem + the explicit "V-B-A before excluding unit / document either way" AC), the test.yml diff, the unit suite's real-content dependencies (grep for readFileSync/spawnSync over learn//resources/content//.md), and the dedicated lint workflows (tree-json-lint.yml, skill-manifest-lint.yml).
• Expected Solution Shape: A per-suite change-detection gate (not workflow-level paths-ignore, which would unreport required checks) that skips heavy suites for docs/content-only diffs — but keeps unit running for every verified unit-spec content dependency, with the V-B-A documented.
• Patch Verdict: Matches the shape and handles the hard parts well. The classifier, the needs: changes gating, the status-preserving skip step, and the release-notes/portal carve-out are all correct. Gap: the content-dependency V-B-A is incomplete (one spec missed).

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #12705
• Related Graph Nodes: tree-json-lint.yml + skill-manifest-lint.yml (dedicated substrate-lint workflows), #12728 (release-notes→SEO routes, why release-notes correctly runs unit), #12704 (the originating friction)

🔬 Depth Floor

Challenge (the blocking gap): requiresUnitForContent covers resources/content/release-notes/ + apps/portal/resources/data/, but misses a real unit-suite content dependency: test/playwright/unit/apps/portal/view/news/discussions/Component.spec.mjs:195 does readFileSync('resources/content/discussions/chunk-2/discussion-11891.md', 'utf8') and feeds it through marked + the component. A manual PR editing resources/content/discussions/** is classified docs-only → run_unit=false → that spec is skipped, and it has no dedicated workflow backstop. #12705's AC explicitly requires this V-B-A to be complete and documented. (Practical risk is low — discussions content normally arrives via the sync pipeline with [skip ci], so the exposed case is a manual edit — but the AC asks for the decision either way.)

Documented search (the part that's clean — credit): I actively checked whether the substrate guards false-skip: lint-tree-json (the learn/tree.json↔folder mirror, the #12238 class), lint-agents (AGENTS.md/learn/agentos/**/skill .md anchor-tags), and lint-skill-manifest all run in dedicated workflows (tree-json-lint.yml on learn/**; skill-manifest-lint.yml on .agents/skills/** + .claude/skills/**), independent of test.yml. So they are not false-skipped by this change — leaving them out of the unit carve-out is correct, not a gap. I also confirmed the changes job fails safe (unavailable files → both suites run) and handles both pull_request and push events.

Rhetorical-Drift Audit:

• PR framing matches the diff.
• Minor drift: the inline comment "Unit has committed content guards, notably the release-note mutable GitHub-link scan and generated portal index fixtures" is accurate but incomplete — it omits the discussions/Component.spec real-content read. Update it (or the carve-out) so the comment reflects the full content-dependency set.

Findings: One blocking carve-out gap + the matching comment-completeness note.

🧠 Graph Ingestion Notes

• [RETROSPECTIVE]: Path-scope-skip safety = enumerate every unit-suite real-content dependency, not just the obvious ones. Two distinct backstops exist here: dedicated lint workflows (tree-json/agents/skill-manifest — safe to skip in test.yml) vs. unit specs that readFileSync real content with no other workflow (release-notes/portal — carved out; discussions/Component — missed). The classifier is only as safe as the completeness of that enumeration.

🎯 Close-Target Audit

• Close-targets identified: #12705
• #12705 confirmed not epic-labeled (enhancement, ai, build)

Findings: Pass.

🧪 Test-Execution & Location Audit

• CI green on the PR (Classify test scope, integration-unified, unit, CodeQL all SUCCESS) — the classifier runs.
• Verification method: source analysis of the classification logic vs. (a) the unit suite's real-content readFileSync dependencies and (b) the dedicated lint workflows. Appropriate for a CI-config change — the risk class is a future false-skip on a docs/content PR, not a current test failure, so green CI here doesn't exercise the gap. No branch checkout needed (workflow-config change).

Findings: Green; the gap is a future-false-skip surfaced by static analysis, in Required Actions.

N/A Audits — 📑 📡 🔗

N/A: no consumed-code API contract / Contract Ledger surface (📑), no openapi.yaml (📡), no new skill/convention/MCP tool (🔗 — this is a CI workflow change; the relevant cross-substrate interaction is the dedicated-lint-workflow split, which is handled correctly).

📋 Required Actions

To proceed with merging, please address the following:

• Close the unit-content-dependency V-B-A (per #12705's AC). discussions/Component.spec.mjs:195 reads real resources/content/discussions/…md. Either (a) extend requiresUnitForContent to cover the resources/content/ subdirs that unit specs read (at minimum resources/content/discussions/), or (b) document in the PR/comment why it's acceptable to skip (e.g., discussions changes only ever arrive via the sync pipeline's [skip ci], so a manual edit is out-of-contract). Either satisfies the AC's "document the finding either way."
• (Trivial) Update the inline requiresUnitForContent comment to reflect the full content-dependency set once (a)/(b) is decided.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 88 — correct per-suite gating (not workflow-level paths-ignore), fail-safe default, status-checks preserved, dedicated-lint-workflow split respected. −12 for the incomplete content-dependency enumeration.
• [CONTENT_COMPLETENESS]: 70 — the carve-out V-B-A (an explicit #12705 AC) misses one real unit-content dependency; otherwise complete.
• [EXECUTION_QUALITY]: 90 — clean github-script, paginated, handles PR+push, fail-safe, status-shim; CI green.
• [PRODUCTIVITY]: 85 — directly removes the docs-PR CI friction from #12704; one cheap cycle-2 to close the gap.
• [IMPACT]: 80 — every docs/content PR stops paying the ~5-11 min heavy-suite wait + the reviewer §7.6 pending-CI dilemma; broad, recurring benefit.
• [COMPLEXITY]: 45 — moderate: change-detection + per-matrix if: gating + the required-check-preservation subtlety.
• [EFFORT_PROFILE]: Quick Win — high-leverage CI ergonomics fix; the remaining work is a one-line carve-out or a documented decision.

Strong change — close the one carve-out gap (or document the skip decision) and I'll re-review promptly. Nice handling of the required-check-preservation gotcha; that's the part most naive paths-ignore attempts get wrong.