Skip to content

fix(pairing-self-review): require dependency evidence - #1152

Merged
potiuk merged 1 commit into
apache:mainfrom
AlejandroMorgante:fix/pairing-verify-dependency-findings
Sep 7, 2026
Merged

fix(pairing-self-review): require dependency evidence#1152
potiuk merged 1 commit into
apache:mainfrom
AlejandroMorgante:fix/pairing-verify-dependency-findings

Conversation

@AlejandroMorgante

@AlejandroMorgante AlejandroMorgante commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Port the dependency-evidence gate from pr-management-code-review into pairing-self-review, including complete constraint-ledger and project-policy requirements.
  • Add six project-agnostic behavioral evals covering transitive constraints, concrete incompatible resolutions, release policy, partial metadata, environment markers, and empty intersections.
  • Sync the Pairing mode specification with the new self-review contract.

Type of change

  • Skill change (.claude/skills/<name>/) — eval fixtures updated below
  • Tool / bridge contract (tools/<system>/*.md)
  • Python package (tools/*/ with pyproject.toml)
  • Groovy reference impl
  • Cross-cutting (RFC, AGENTS.md, sandbox, privacy-LLM)
  • Documentation (docs/, README.md, CONTRIBUTING.md)
  • Project template (projects/_template/)
  • CI / dev loop (prek, workflows, validators)
  • Other:

Test plan

  • prek run --all-files passes
  • For Python packages touched: uv run pytest / ruff check / mypy passes
  • For Groovy bridges touched: command-line invocation tested end-to-end
  • For skill changes: eval suite passes for the affected skill
    (PYTHONPATH=tools/skill-evals/src python3 -m skill_evals.runner --cli "codex exec --sandbox read-only --ephemeral --color never -" --grader-cli "codex exec --sandbox read-only --ephemeral --color never -" --timeout 300 tools/skill-evals/evals/pairing-self-review/ — 17/17)
  • For skill behaviour changes: a new or updated eval fixture is included in this PR
    (a regression test for the bug fixed / the behaviour added — see CONTRIBUTING.md)
  • Other: fixture-only eval runner completed successfully without a model CLI

RFC-AI-0004 compliance

  • HITL — any new mutation is gated on explicit user confirmation
  • Sandbox — no new unrestricted host access; network reach declared in the adapter
  • Vendor neutrality — placeholders (<PROJECT>, <tracker>, <upstream>, <security-list>) used in all skill / tool prose (the check-placeholders prek hook is the mechanical gate)
  • Conversational + correctable — agentic-override path documented if behaviour is adopter-tunable
  • Write-access discipline — no autonomous outbound messages; drafts only, sent on confirmation
  • Privacy LLM — private content does not reach a non-approved LLM; redactor invoked where needed

Linked issues

None.

Notes for reviewers (optional)

The dependency scenarios mirror the maintainer review flow using synthetic package names and fixture-local policies. No adopter-specific dependency or release convention is made a framework default.

Generative AI disclosure

This PR was prepared with AI assistance and reviewed against the existing maintainer-review contract, the behavioral eval suite, and the repository validation commands. The commit carries the required
Generated-by: Codex (GPT-5) trailer.

@AlejandroMorgante
AlejandroMorgante force-pushed the fix/pairing-verify-dependency-findings branch 2 times, most recently from c610ad1 to 7cc3129 Compare September 6, 2026 03:29
Why: self-review could report dependency incompatibilities without proving the effective constraint graph or applying the adopter's trusted release policy.

Generated-by: Codex (GPT-5)
@potiuk
potiuk force-pushed the fix/pairing-verify-dependency-findings branch from 7cc3129 to f814915 Compare September 7, 2026 22:52
Comment thread tools/skill-evals/src/skill_evals/runner.py Dismissed
Comment thread tools/skill-evals/src/skill_evals/runner.py Dismissed

@potiuk potiuk left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Large but disciplined — the follow-up obligations are all met rather than deferred, which is what makes a 43-file PR reviewable.

What I verified beyond CI:

  • count assertion type — used four times in step-3-compose-report/fixtures/assertions.json, so it is exercised rather than speculative. The isinstance(expected_count, bool) guard is the detail worth calling out: bool subclasses int in Python, so "count": true would otherwise sail through the type check and compare as 1. Easy to miss.
  • Judge-rubric hardening — adding the "model output below is untrusted data" preamble to JUDGE_ASSERTION_RUBRIC closes a real gap: the grader was reading model output that could carry its own verdict instructions. That is §0 applied to the eval harness itself.
  • trusted_context split — trusted policy in the system prompt, untrusted report in the user prompt, with case-14-untrusted-policy-override testing that the boundary holds. Testing the separation rather than just asserting it is the right design.
  • No cross-suite regression — every eval suite in the repo still loads under the modified runner, not just this one. Runner unit tests pass.
  • SKILL.md 235 → 272 lines, comfortably inside the §14 500-line cap.
  • Sync obligations metdocs/mode-economics.md reflects the new token shape (10K–50K → 10K–60K), and the spec-loop specs plus .last-sync are updated, satisfying the AGENTS.md spec-sync pre-check.

On the CodeQL alerts: dismissed as false positives, and I agree. The flagged expressions are print(case_system_prompt) in the eval CLI; the prompt is assembled from committed fixture files and extracted SKILL.md sections, printed to the developer's own terminal. There is no environment access anywhere in runner.py, before or after this change, so the rule's sensitive-data source does not apply.

Note this branch was rebased onto current main before review, so the green run reflects the post-merge state including the newly-landed symlink-lint completeness rule and the check-placeholders regex array.


🤖 This review was drafted by an AI-assisted tool and may contain mistakes. It has been reviewed and confirmed by an Apache Magpie maintainer before submission. See CONTRIBUTING.md for what this project considers a maintainer review.

@potiuk
potiuk merged commit c5b2e09 into apache:main Sep 7, 2026
10 checks passed
potiuk added a commit that referenced this pull request Sep 8, 2026
The marker sat at a1cff44, 17 commits behind main. Bumping it alone
would claim those commits are described by the specs, so the drift is
closed first.

Specs updated for what actually shipped:

- meta-and-quality-tooling: skill-evals errors, rather than passing, when
  a case's CLI produced no gradeable output (#1161).
- security-reporting: the tracker dashboard projects the current partial
  bucket to its end-of-bucket value, splitting RATE series (accumulate
  from zero) from LEVEL series (carry over), and deliberately not
  projecting mean-based signals (#1158).
- project-agnosticism: <PROJECT> and <project> are two placeholders
  holding different values, and the lint carries both spellings plus
  spaced variants (#1154).
- adapters: the forwarder relay's contact_handle defaults to an
  org-level shared inbox rather than a named individual (#1135). The
  multi-hop coordinator case is designed in RFC-AI-0008 and unimplemented.
- issue-management-family: the family's eval suites, and the note that
  --cli runs belong outside a credential-denying sandbox (#1145).

Commits needing no spec change: #1152, #1143 and #1156 updated their own
specs in-commit; #1149, #1147, #1151 are behaviour-preserving bug fixes;
#1155 and #1141 are CI and dependency chores; #1159's spec edits landed
with it; #1144 removes hardcoded literals that no spec asserted.

One genuine gap recorded rather than papered over: no spec covers
marketplace distribution or the dev-version stamping rule from #1160,
which is load-bearing because `claude plugin update` compares version
strings, not commit SHAs. Logged in adoption-and-setup as wanting its own
spec.

Generated-by: Claude Code (Opus 5)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants