Skip to content

test(planning): split interview session-config eval into engineering + general session cases - #1092

Merged
kyle-sexton merged 4 commits into
mainfrom
fix/1083-planning-interview-eval-framing
Jul 26, 2026
Merged

test(planning): split interview session-config eval into engineering + general session cases#1092
kyle-sexton merged 4 commits into
mainfrom
fix/1083-planning-interview-eval-framing

Conversation

@kyle-sexton

@kyle-sexton kyle-sexton commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

Summary

The interview skill's recommends-session-config eval only checked the "downstream execution session" framing, so a general/terminal-session case could still pass on that stale framing after the skill contract moved general sessions to recommend config for the current/next session. This splits the eval into an engineering/downstream case and a general/current-session case so both framings are actually covered.

Dependency on #848 — satisfied

This depended on #848 (docs(planning): reframe interview session-config framing), which merged 2026-07-25 (bc86410). Main's SKILL.md / context/session-config.md now carry the current/next-session framing eval 10 asserts, including the stop-boundary refresh framed as current/next-session config, so the skill and the eval are consistent. main was merged into this branch after #848 landed; the version bump was recomposed over #848's landed 0.26.1 as 0.26.2 (the original 0.25.2 plan predated intervening planning releases).

Fix

plugins/planning/skills/interview/evals/evals.json:

  • Eval 9 renamed recommends-session-config-from-live-docsrecommends-session-config-engineering-downstream. Its prompt is already an engineering task (a code re-architecture), for which the downstream-execution-session framing remains correct; the expected_output and expectations now make that engineering/handoff scope explicit so it is unambiguously the engineering half of the split.
  • New eval 10 recommends-session-config-general-current-session: a clearly non-build general decision (org-structure strategy across ~200 people, with an explicit "not a code change" override to lock the general classification). It asserts the skill recommends config for the current/next session, applied now — not a downstream execution session that will never exist — while keeping the same capability-vs-thoroughness distinction, advisor pairing, live-doc sourcing, and graceful-degrade expectations.

This closes the coverage gap: the eval set can no longer accept downstream-only framing for a general-session case.

Version bump 0.26.1 → 0.26.2 and a [0.26.2] CHANGELOG entry composed above #848's [0.26.1].

Verification

This repo ships no model-graded eval runnerskill-quality:check's own SKILL.md states "Not for: running model-graded evals." So this cannot assert a behavioral pass of the skill against the evals; the available evidence is schema conformance + the static skill gate:

  • check-jsonschema --schemafile plugins/skill-quality/reference/evals.schema.json plugins/planning/skills/interview/evals/evals.jsonok -- validation done
  • python -m jsonschema validate against the same schema → SCHEMA VALID, 10 evals, all id/name unique. Re-validated post-merge against the schema now on main.
  • CHECK_SKILL_SKILLS_ROOT=…/plugins/planning/skills check-skill.sh interviewPASS — 0 errors, 2 warning(s). Both warnings are pre-existing and unrelated (soft line-count target; description trigger phrasing) — the same two docs(planning): reframe interview session-config framing #848's own verification reported; this change introduces none. The run's INFO lines confirm it validated interview (not a no-op).
  • Post-docs(planning): reframe interview session-config framing #848-merge: confirmed eval 10's assertions match the merged skill text on main (early post-survey surface for general sessions + stop-boundary refresh framed as current/next-session config).

Related

Closes #1083

Work-class: C3 (bug-fix-shaped) — attended triage 2026-07-23, operator-ratified. 🤖

…+ general cases

The recommends-session-config eval only covered the downstream-execution-session
framing. Split it so eval 9 explicitly asserts the engineering/downstream case and
new eval 10 asserts the general/terminal case — recommending config for the
current/next session, applied now, since a general session has no downstream
consumer. Closes the coverage gap where a general-session case could still pass on
stale downstream-only framing.

Closes #1083
… split

Serializes behind #848 (still open): #848 bumps to 0.25.1, this lands one past.

Refs #1083
@kyle-sexton kyle-sexton added do-not-merge Hard merge gate: do not merge while applied. automated Opened by automation. labels Jul 23, 2026
@claude

claude Bot commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

Claude encountered an error after 0s —— View job


I'll analyze this and get back to you.

@claude

claude Bot commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

Claude encountered an error after 1s —— View job


I'll analyze this and get back to you.

@github-actions

github-actions Bot commented Jul 23, 2026

Copy link
Copy Markdown

Warning

Automated security review did not complete — this is an infrastructure failure, not a review verdict.

Treat any Claude comment on this PR (including a placeholder like "I'll analyze this and get back to you") as incomplete, not "no findings."

Re-running the job, or pushing a new commit, will retry the review.

@github-actions

github-actions Bot commented Jul 23, 2026

Copy link
Copy Markdown

Warning

Automated review did not complete — this is an infrastructure failure, not a review verdict.

Treat any Claude comment on this PR (including a placeholder like "I'll analyze this and get back to you") as incomplete, not "no findings."

Re-running the job, or pushing a new commit, will retry the review.

@kyle-sexton

Copy link
Copy Markdown
Contributor Author

🔒 babysit-prs lane claiming this PR for on-branch fix work this cycle (amendment-round: 16, safe tier). Will fix clear branch-owned findings and push; will not resolve threads or merge (safe tier).

@kyle-sexton kyle-sexton added agent-ready Fully specified and briefed; eligible for autonomous pickup from the frontier. and removed do-not-merge Hard merge gate: do not merge while applied. labels Jul 23, 2026
@kyle-sexton

kyle-sexton commented Jul 25, 2026

Copy link
Copy Markdown
Contributor Author

Merge-order escalation — held in draft, needs a human decision

Re-verified the ordering constraint stated in this PR's body against current origin/main
(12ec6ff5). It still holds, and the dependency PR turns out to be unmergeable in its current
form, so the "just wait for #848" path is not available without a human decision.

1. The semantic dependency is real and still live

origin/main's plugins/planning/skills/interview/SKILL.md (§ Session-config recommendation)
still frames the recommendation as configuring the downstream execution session and states it is
"applicable to engineering and general sessions alike". context/session-config.md on main carries
the same downstream-only framing.

This branch's eval 10 (recommends-session-config-general-current-session) asserts the opposite
for a general/terminal session — current/next session, applied now, explicitly not a downstream
execution session. Merging this first ships an eval asserting behavior the shipped skill contradicts.

Nothing in CI catches this: the repo ships no model-graded eval runner, so the eval set is inert
data and every gate is green. The contradiction would land silently.

2. The version chain is now stale on both PRs — and this PR has gone DIRTY

main moved to planning 0.26.0 while this was being diagnosed, invalidating the chain both PRs
were calibrated on. Current state:

This branch is consequently DIRTY: git merge-tree against current main conflicts on
plugins/planning/.claude-plugin/plugin.json and plugins/planning/CHANGELOG.md. Nothing else
conflicts — evals.json, the actual substance of this PR, merges clean.

The conflict is deliberately left unresolved, because the correct version is not determined
until the decision below is made — it differs per option:

Under Option A the merge must be redone against a main that already contains #848 regardless, so
resolving now buys nothing durable. main is also moving fast enough that any number chosen now may
be stale again before merge. Re-derive the bump from main at the moment the chosen option is
executed.

3. #848 cannot currently merge — verified-signature trap

main requires verified signatures. #848 carries commit f0aff60a with committer email
ksextonmelodic@gmail.com, which GitHub reports as verified: false, reason: no_user. That is why
#848 sits BLOCKED with 0 unresolved review threads and every check green. The #848 lane reached the
same diagnosis independently and confirmed it empirically — a squash attempt is rejected with
"base branch policy prohibits the merge", and pushing further signed commits does not clear it
(8 verified commits already follow the unverified one). Details:
#848 (comment)

For contrast, both commits on this branch (d39d7b2d, 5efddd3c) are verified: true /
reason: valid with committer 153232337+kyle-sexton@users.noreply.github.com.

4. Labels corrected

do-not-merge had been removed from this PR by kyle-sexton at 2026-07-23T07:36:10Z with no
accompanying comment. It has been re-added, and agent-ready swapped for needs-human.

Rationale: draft status alone is not a machine-enforced brake. This PR was CLEAN with all required
checks green, 0 unresolved threads, and verified commits — an autonomous drain lane reading only that
state would reasonably judge it "genuinely complete", flip it ready, and merge, without ever reading
this comment. do-not-merge is a required check, so it is the only signal the merge gate actually
enforces; a human clears it in one click. needs-human matches its own description exactly
("Human-in-the-loop required; autonomous sessions must not resolve items carrying this") and is
already on #848 for this same blocker.

The decision

Option A — unblock #848 first, then merge this (preserves the authored plan).
The cheapest route, surfaced by the #848 lane: f0aff60a is signed with the account's usual
ed25519 SSH signing key; only the committer email fails to map to a GitHub user, and the commit has
no persisted verification record (verified_at: null — GitHub persists only verified records). So
adding ksextonmelodic@gmail.com as a verified email on the kyle-sexton account should let GitHub
re-evaluate the commit as Verified with zero history change. Likely but not doc-guaranteed;
trivially checkable afterward via the commits API. Failing that, recreate #848's branch with
correctly attributed commits. A history rewrite / force-push is forbidden by the operating
constraints here.

#848 has already re-synced itself to 0.26.1 and is green on every check, so this option is now
one human action from merge-ready. After it lands: merge main into this branch, take 0.26.2,
mark ready, merge.

Option B — merge this ahead of #848.
Accepts a transient SKILL.md-vs-eval-10 contradiction on main, plus a forced re-bump and conflict
resolution on #848. Cheapest to execute, worst end state — and note it stands in direct tension with
§1: merging off main at 0.26.0 lands an eval asserting framing main does not yet carry, and
because no model-graded runner exists the contradiction sits on main silently until #848 lands.
Making B genuinely clean means either weakening eval 10 to the framing main actually has — which
re-opens #1083, the exact coverage gap this PR exists to close — or folding the reframe in, at which
point B has converged on Option C. So B is only attractive if the goal is to clear this PR from the
queue and carry the contradiction as an explicitly tracked, time-boxed debt.

Option C — fold the reframe into this PR and supersede #848.
Land #848's SKILL.md / context/session-config.md reframe as fresh, correctly-attributed commits
on this branch
, then close #848 as superseded. This is not a history rewrite — new commits with new
content — so the f0aff60a trap is sidestepped entirely, skill and eval land together and
self-consistently, and the version chain collapses to a single bump off current main. It expands this PR's scope and
retires another PR's work, so it is emphatically a human call, but it is the only path that unties
the knot without depending on the account-email fix landing.

@kyle-sexton kyle-sexton added do-not-merge Hard merge gate: do not merge while applied. needs-human Human-in-the-loop required; autonomous sessions must not resolve items carrying this. and removed agent-ready Fully specified and briefed; eligible for autonomous pickup from the frontier. labels Jul 25, 2026
Compose the eval-split version bump over main's 0.26.1 (from #848): plugin
version 0.26.2, CHANGELOG entry [0.26.2] above [0.26.1].
@claude

claude Bot commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

Claude encountered an error after 2s —— View job


I'll analyze this and get back to you.

@claude

claude Bot commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

Claude encountered an error after 2s —— View job


I'll analyze this and get back to you.

@kyle-sexton kyle-sexton removed do-not-merge Hard merge gate: do not merge while applied. needs-human Human-in-the-loop required; autonomous sessions must not resolve items carrying this. labels Jul 26, 2026
@kyle-sexton
kyle-sexton marked this pull request as ready for review July 26, 2026 00:30
@claude

claude Bot commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

Claude encountered an error after 2s —— View job


I'll analyze this and get back to you.

@claude

claude Bot commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

Claude encountered an error after 2s —— View job


I'll analyze this and get back to you.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 5b69a1ee41

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread plugins/planning/skills/interview/evals/evals.json Outdated
…ry refresh timing

Codex P2 on eval 10: the merged skill contract (SKILL.md session-config
section) requires a general/terminal session's first config read right after
the Step 1 survey classifies the domain as general, with the stop-boundary
recommendation as a refresh and a re-evaluate offer when config was raised
only at the end. The eval asserted only the stop-boundary framing, so a
regression that emits the recommendation only after the rounds would still
pass. Eval 10's expected_output and expectations now require the early
surfacing, the refresh framing, and the re-evaluate offer.
@claude

claude Bot commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

Claude encountered an error after 2s —— View job


I'll analyze this and get back to you.

@claude

claude Bot commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

Claude encountered an error after 2s —— View job


I'll analyze this and get back to you.

@kyle-sexton
kyle-sexton merged commit 72e77b6 into main Jul 26, 2026
26 checks passed
@kyle-sexton
kyle-sexton deleted the fix/1083-planning-interview-eval-framing branch July 26, 2026 00:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

automated Opened by automation.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

planning/interview: recommends-session-config-from-live-docs eval still asserts old downstream-only framing

1 participant