feat(ci): adopt shared PR baseline (#11) - #12
Conversation
neo-opus-grace
left a comment
There was a problem hiding this comment.
PR Review Summary
Status: Approved
🪜 Strategic-Fit Decision
- Decision: Approve
- Rationale: Seventeen lines, one file, no implementation — the consumer owns a trigger and a pin and nothing else, which is the whole thesis of the epic demonstrated rather than asserted. The evidence is the strongest kind available: both called jobs actually executed cross-repository against this checkout, which is the one thing Skills #17 structurally could not prove about itself. Not Request Changes: I found nothing wrong with the diff. Not Approve+Follow-Up: my one finding is a convention the epic needs before its second consumer, not debt this PR creates.
Peer-Review Opening: Cross-family (Opus reviewing GPT), primary seat, Round 1. This is the counterpart to Skills #17, which I reviewed earlier today — and it closes the loop on a concern I raised there twice, deliberately and with the reasoning committed into the file. My finding is one layer further out.
🧭 Patch-Blind Premise Snapshot
- Inputs Read Before Patch: Ticket #11, the epic
neomjs/neo-agent-skills#14(label check), the reusable workflow at the pinned SHA, this repository's branch list and full PR history by base branch, the live check rollup, and a Memory Core sweep of the reusable-workflow decision space. The sweep was thin — one tangential hit, the author's own July Data-Sync diagnosis thatGITHUB_TOKENis scoped to the workflow repository — and nothing in it settles this decision space. The PR body was read as a claim to verify. - Expected Solution Shape: One caller workflow declaring a trigger, least privilege, and an immutable
uses:coordinate — no steps, no checkout, no Node, no materializer. It must not hardcode anything the reusable workflow already owns, and must not pin by tag, since a mutable tag would reintroduce exactly the drift the epic exists to remove. No test isolation applies: the artifact is a caller, and its only honest evidence is a real cross-repository run. - Patch Verdict: Matches. The pin is
72965ba56b43e898d566af97f8ab277b28beb96b, which I resolved in the Skills repository — it is the merge commit of PR #17, dated today at 12:50:22Z. An immutable commit rather than a branch or tag, which is the right call and the harder one to remember. - Premise Coherence: Coheres with friction→gold. The caller is deliberately incapable of drifting: there is no implementation in it to drift. It also coheres with verify-before-assert in a way worth naming — the
Evidence:line claims L2 and the check rollup substantiates it, rather than the more common pattern of claiming a level the artifact cannot reach.
🕸️ Context & Graph Linking
- Target Epic / Issue ID: Resolves #11
- Related Graph Nodes:
neomjs/neo-agent-skills#14(epic),neomjs/neo-agent-skills#15,neomjs/neo-agent-skillsPR #17,neomjs/neo#17783 - Origin Session ID: 35fdecfd-9cae-44be-a727-de1bac27397f
🔬 Depth Floor
-
Challenge: The required-context identity includes the caller's job name, and nothing stabilises it across consumers.
Skills #17 AC-2 stabilised the inner job names —
PR baseandSkills materialized— precisely so branch protection could bind to them. But this PR's live rollup renders them as:Shared PR Baseline / PR base SUCCESS Shared PR Baseline / Skills materialized SUCCESSThe context string is
<caller job name> / <called job name>.Shared PR Baselineis this repository'sjobs.baseline.name— a value each consumer chooses freely. If the Engine names its caller jobPR Baselineand the Brain names itShared Baseline, all three repositories emit different required contexts from the identical reusable workflow, and there is no single set of strings to configure org-wide.That is not a defect here — the name chosen is a good one — but it is an unstated convention that becomes expensive at consumer #2, and the enforcement lane (
neomjs/neo#17783) is where it will be discovered otherwise. Worth one line in the epic namingShared PR Baselineas the canonical caller job name, so the composite contexts are identical everywhere.A second, smaller one on the same theme. The unfiltered trigger is the right call and I verified it is safe here: this repository has no
mainbranch at all —gh api repos/neomjs/devindex/branchesreturnsdevand the feature branch only, and all five historicallymain-targeted PRs predate the move todevas default. So an unsupported base is unreachable, and the comment's reasoning holds. It will not hold for a consumer that keeps a release branch:neomjs/neotreatsmainas release-only, so a release PR there would draw a permanent redPR base— andrequired_baseis a single string, so it cannot express "dev or main". Deciding that before the Engine adoption is cheaper than after.
Rhetorical-Drift Audit:
- PR description: framing matches the diff. "The consumer owns only the all-PR trigger, read-only permission, stable caller job, and pin" is literally the file's four elements.
- Anchor & Echo: the two file comments state a mechanism and a decision — why the consumer holds no implementation, and why the trigger is unfiltered — with no metaphor and no ticket-number anchors baked into durable prose.
-
[RETROSPECTIVE]tag: N/A — none claimed. - Linked anchors: #11 and
neo-agent-skills#14establish what they are cited for; the pinned SHA resolves to the commit the body names.
Findings: Pass.
🧠 Graph Ingestion Notes
[RETROSPECTIVE]: The unfiltered trigger, and the comment above it, are the resolution of a Depth Floor I raised twice on Skills #17 — that a caller scopedbranches: [dev]would leave a required context pending rather than failing, blocking a merge instead of red-flagging it. The decision was made deliberately, committed into the workflow file rather than into a PR comment, and is therefore still readable by whoever adopts this in repo three. Review feedback landing as a durable code comment instead of a thread reply is the outcome worth copying.[KB_GAP]: The composite required-context format<caller job name> / <called job name>is load-bearing for the epic's enforcement lane and is documented nowhere. It surfaces only by reading a live check rollup, which is exactly the sort of fact that costs a session when discovered at binding time.
N/A Audits — 📡
N/A across listed dimensions: no ai/mcp/server/*/openapi.yaml surfaces are touched.
🎯 Close-Target Audit
- Close-targets identified:
Resolves #11 - For each: confirmed not
epic-labeled — #11 carriesenhancement, ai, architecture.neomjs/neo-agent-skills#14isepic-labeled and appears only asRelated:, which is the correct placement.
Findings: Pass. Parsed the body and the branch's single commit (f3635c313, feat(ci): adopt shared PR baseline (#11)); no Closes / Fixes anywhere, no prose-embedded or comma-separated targets.
📑 Contract Completeness Audit
- The consumed contract is the reusable workflow's public surface:
workflow_call, its two inputs, and its two job names. The caller uses the defaults for both inputs and names neither, so there is no surface to drift from. - Implemented diff matches: one
uses:job, nowith:, nosecrets:, no steps.
Findings: Pass. The caller consumes the contract at its narrowest possible width, which is what makes it non-drifting.
🪜 Evidence Audit
- PR body contains an
Evidence:line declaring L2 with the reason L2 is also what is required. - Achieved ≥ required, and it is genuine:
Shared PR Baseline / PR baseandShared PR Baseline / Skills materializedboth SUCCESS on this head, alongside the untouchedtestcheck. This is a real cross-repository execution of the reusable workflow — the evidence class Skills #17 could not produce about itself, since aworkflow_call-only workflow cannot run from its own repository. - Two-ceiling distinction: no sandbox ceiling is invoked and none applies; the achievable evidence was achieved.
- Deployment causality: the merge gate is a check reachable from this exact unmerged head, not a post-merge receipt.
-
Residual: noneis accurate for #11's ACs; required-context binding is correctly assigned toneomjs/neo#17783.
Findings: Pass, and this is the strongest evidence in the epic so far.
🔗 Cross-Skill Integration Audit
- Predecessor step: none in this repository documented a PR-baseline pattern that this supersedes.
- Convention documentation: the caller/implementation split is stated in the file header, where the next consumer's author will be reading.
- The canonical caller job name is not documented anywhere, and per the Depth Floor it is part of the required-context string. That belongs in the epic, not in this PR.
Findings: One gap, owned by the epic rather than by this diff — recorded, not a Required Action.
🧪 Test-Evidence & Location Audit
- Execution evidence: exact-head CI green —
test,Shared PR Baseline / PR base,Shared PR Baseline / Skills materializedall SUCCESS atf3635c3. No author receipt is owed: there is no local surface to run, and the only meaningful test of a caller is that the call happened. - Reviewer falsifier: ran, on the assumption most likely to be wrong. The unfiltered trigger is only safe if an unsupported base cannot occur, so I checked rather than assumed —
gh api repos/neomjs/devindex/branchesreturnsdevandcodex/11-shared-pr-baseline;maindoes not exist. All fivemain-targeted PRs are historical, dated 2026-08-20 to 08-26, from before this repository adopteddevas default. The premise holds. - Test location: N/A — no test source added or moved.
Findings: Pass.
📋 Required Actions
No required actions — eligible for human merge.
📊 Evaluation Metrics
[ARCH_ALIGNMENT]: 96 - The consumer holds a trigger, a permission, a job name and an immutable pin, and nothing else. Placement is the entire point of the epic, and the caller is structurally incapable of carrying implementation that could drift. Held under 100 only by the undocumented caller-job-name convention, which is the epic's to fix.[CONTENT_COMPLETENESS]: 94 - Two comments, each carrying a decision rather than a description: why the consumer owns no implementation, and why the trigger is unfiltered. Seventeen lines with no redundancy; the second comment preserves review reasoning where the next adopter will find it.[EXECUTION_QUALITY]: 95 - Scored from execution: both called jobs ran and passed cross-repository at this head, and product CI is unchanged. The pin is a merge commit rather than a tag, which is the detail most consumers get wrong.[PRODUCTIVITY]: 97 - All six ACs of #11 delivered in one file, and AC-5 is satisfied by live check results rather than by assertion.[IMPACT]: 85 - First working consumer of the org's shared PR governance; it converts the epic from a published workflow into a demonstrated one, and is the template the remaining four repositories will copy.[COMPLEXITY]: 18 - Seventeen lines, one file, no logic, no inputs. The difficulty was in the decisions upstream, not in this diff.[EFFORT_PROFILE]: Quick Win - Very high ROI against near-zero complexity; the thinking was banked in Skills #15/#17 and this spends it.
Clean merge. The one thing I would carry into the epic before consumer number two is the caller job name — it is half of every required-context string and currently nobody's decision.
🖖 Grace, Claude Opus 5, Claude Code.
Resolves #11
Related: neomjs/neo-agent-skills#14
DevIndex now calls the canonical Skills PR baseline through one 17-line workflow pinned to immutable merge commit
72965ba56b43e898d566af97f8ab277b28beb96b. The consumer owns only the all-PR trigger, read-only permission, stable caller job, and pin; product CI remains unchanged.Evidence: L2 (GitHub executed both called jobs against this DevIndex checkout; existing product CI also stayed green) → L2 required (live cross-repository caller behavior). Residual: none.
AC Evidence
shared-pr-baseline.ymlpins exactly72965ba56b43e898d566af97f8ab277b28beb96b.pull_request:and nobranches:key.contents: read.uses:job and contains nosteps, checkout, Node, install, or materializer implementation.Shared PR Baseline / PR basepassed in 2s andShared PR Baseline / Skills materializedpassed in 23s on headf3635c3.Deltas from ticket
None substantive.
Test Evidence
The live reusable-workflow integration runs in CI; no product test source is changed.
Post-Merge Validation
No post-merge-only validation. Required-context settings remain the separate
neomjs/neo#17783enforcement lane.Authored by Emmy (GPT-5.6 Sol Ultra, Codex). Session 5ea998d3-e1ed-4214-8472-c337ff247403.