diff --git a/.agents/skills/apm-review-panel/SKILL.md b/.agents/skills/apm-review-panel/SKILL.md deleted file mode 100644 index c44babfc..00000000 --- a/.agents/skills/apm-review-panel/SKILL.md +++ /dev/null @@ -1,415 +0,0 @@ ---- -name: apm-review-panel -description: >- - Use this skill to run a multi-persona expert advisory review on a labelled - pull request in microsoft/apm. The panel fans out to five mandatory - specialists plus a test-coverage specialist (active on every PR that - touches src/) plus two conditional specialists (auth, doc-writer), - all running in their own agent threads, and a CEO - synthesizer. The orchestrator is the sole writer to the PR: ONE - recommendation comment, no verdict labels, no merge gating. The panel - is advisory -- it surfaces findings, prioritizes follow-ups, and renders - a ship-recommendation that the maintainer and author weigh. Activate - when a non-trivial PR needs a cross-cutting recommendation - (architecture, CLI logging, DevX UX, supply-chain security, - growth/positioning, optionally auth, docs, and test coverage, with CEO - arbitration). ---- - -# APM Review Panel - Fan-Out Advisory Review - -The panel is FAN-OUT + SYNTHESIZER. Each persona runs in its own agent -thread (via the `task` tool) and returns JSON matching -`assets/panelist-return-schema.json`. The orchestrator schema-validates -each return, hands all returns to the apm-ceo synthesizer (also a task -thread, returns JSON matching `assets/ceo-return-schema.json`), then -renders ONE recommendation comment from `assets/recommendation-template.md`. - -This skill is ADVISORY by design. It does not compute a binary verdict, it -does not apply verdict labels, and it does not gate merge. The panel -surfaces findings; the maintainer and the PR author decide ship. - -## Architecture invariants - -- **Advisory regime, not gate regime.** There is no `APPROVE` / `REJECT`, - no `panel-approved` / `panel-rejected` label, no deterministic verdict - computation. The CEO returns a `ship_recommendation.stance` (`ship_now` - / `ship_with_followups` / `needs_discussion` / `needs_rework`); this is - prose for the human reviewer, never auto-applied as a label or status - check. This is the architectural fix for the previous regime's - over-strictness: removing the binary gate removes the incentive for - panelists to inflate `required[]` defensively. -- **Three severity buckets, none of them gate.** Findings carry - `severity: blocking | recommended | nit`. `blocking` is the highest - signal a panelist can send and renders prominently in the comment; it - still does not block merge. `recommended` is the default for substantive - feedback. `nit` is one-line polish. The orchestrator never reads - severity to gate anything. -- **Single-writer interlock.** Only the orchestrator writes to the PR: - exactly one `add-comment` and one `remove-labels` call. The - `remove-labels` call always sweeps `panel-review` (trigger - idempotency) AND defensively removes `panel-approved` / - `panel-rejected` if present (legacy verdict labels from the - pre-advisory regime; they have no meaning here and would mislead - readers if left on a PR after a fresh advisory pass). NO `add-labels` - call -- there are no verdict labels to apply. Panelist subagents and - the CEO subagent return JSON only and MUST NOT call any `gh` write - command, post comments, apply labels, or touch the PR state. -- **Single-emission discipline.** Exactly one comment per panel run, - rendered from `assets/recommendation-template.md` after all subagents - return. - -## Agent roster - -| Agent | Role | Always active? | -|-------|------|----------------| -| [Python Architect](../../agents/python-architect.agent.md) | Architectural Reviewer + supplies mermaid diagrams | Yes | -| [CLI Logging Expert](../../agents/cli-logging-expert.agent.md) | Output UX Reviewer | Yes | -| [DevX UX Expert](../../agents/devx-ux-expert.agent.md) | Package-Manager UX | Yes | -| [Supply Chain Security Expert](../../agents/supply-chain-security-expert.agent.md) | Threat-Model Reviewer | Yes | -| [OSS Growth Hacker](../../agents/oss-growth-hacker.agent.md) | Adoption Strategist | Yes | -| [Auth Expert](../../agents/auth-expert.agent.md) | Auth / Token Reviewer | Conditional (see below) | -| [Doc Writer](../../agents/doc-writer.agent.md) | Documentation Reviewer | Conditional (see below) | -| [Test Coverage Expert](../../agents/test-coverage-expert.agent.md) | Test-Presence Reviewer (paired with DevX UX) | Yes (skipped only on docs-only PRs -- see below) | -| [APM CEO](../../agents/apm-ceo.agent.md) | Strategic Arbiter / Synthesizer | Yes | - -## Topology - -``` - apm-review-panel SKILL (orchestrator thread) - | - FAN-OUT via task tool (panelists in parallel) - | - +-----+-------+-------+-----+-----+------+-----------+----------+ - v v v v v v v v v (cond.) - py cli dx-ux sec grw auth doc-writer test-cov - | | | | | | | | - | each returns JSON per panelist-return-schema.json - +-----+-------+-------+-----+-----+------+-----------+----------+ - | - v <-- S4 schema-validate - v <-- on malformed: re-spawn that persona - v - task: apm-ceo synthesizer - - aggregates findings across panelists - - resolves dissent - - emits headline + arbitration prose + principle alignment - - emits curated recommended_followups (prioritized) - - emits ship_recommendation (stance + prose) - - returns ceo-return-schema.json - | - v <-- S4 schema-validate - v - orchestrator (sole writer) - | | - v v - add-comment remove-labels - (max:2) [panel-review, - panel-approved, - panel-rejected] - (trigger reset + - legacy verdict sweep) -``` - -## Conditional panelists - -Two personas are conditional (auth, doc-writer). A third -(test-coverage) is mandatory on every PR that touches `src/` and only -skipped on documentation-only PRs -- see its section below for why. -The orchestrator ALWAYS spawns ALL three tasks to keep the schema -return shape uniform; the prompt instructs the subagent to set -`active: false` with an `inactive_reason` if the condition does not -hold. - -### Auth Expert - -Activate when the PR changes any of: -- `src/apm_cli/core/auth.py` -- `src/apm_cli/core/token_manager.py` -- `src/apm_cli/core/azure_cli.py` -- `src/apm_cli/deps/github_downloader.py` -- `src/apm_cli/marketplace/client.py` -- `src/apm_cli/utils/github_host.py` -- `src/apm_cli/install/validation.py` -- `src/apm_cli/install/pipeline.py` -- `src/apm_cli/deps/registry_proxy.py` - -Fallback self-check (when no fast-path file matched): "Does this PR -change authentication behavior, token management, credential resolution, -host classification used by `AuthResolver`, git or HTTP authorization -headers, or remote-host fallback semantics? If unsure, answer YES." - -### Doc Writer - -Activate when the PR changes any of: -- `README.md` -- `CHANGELOG.md` -- `MANIFESTO.md` -- `docs/src/content/docs/**` -- `.apm/skills/**/*.md` -- `.apm/agents/**/*.md` -- `.github/skills/**/*.md` -- `.github/agents/**/*.md` -- `.github/instructions/**/*.md` -- `.github/workflows/*.md` (gh-aw natural-language workflows) -- `packages/apm-guide/**` - -Fallback self-check (when no fast-path file matched): "Does this PR -change user-facing documentation, agent or skill prose, instruction -files, CHANGELOG entries, README claims, or any natural-language -artifact a reader will rely on? If unsure, answer YES." - -When the doc-writer is active and the PR includes documentation changes, -the persona reviews them for: (a) consistency with the existing voice -and structure, (b) accuracy against the code being changed, (c) -completeness for the typical reader (no orphan claims, no missing -prerequisites), (d) discoverability (cross-links, sidebar order if -Starlight content). When the doc-writer is active because of code -changes that SHOULD have updated docs but did not, the persona surfaces -that gap as a finding. - -### Test Coverage Expert - -**Active by default on every PR that touches `src/**/*.py`.** The only -condition that flips this persona to `active: false` is a -documentation-only PR -- the diff contains zero `src/**/*.py` files. -In that case set `inactive_reason: "documentation-only PR -- no -runtime code paths to defend"`. - -The activation rule is intentionally narrow: under the advisory regime, -test outcomes are LOAD-BEARING for CEO arbitration (passed / failed / -missing test evidence outranks opinion-only findings -- see -`apm-ceo.agent.md` and `panelist-return-schema.json` evidence block). -A persona whose findings carry that weight cannot be silently skipped -on a heuristic. Better to spawn it on a pure refactor and have it -return a single `nit`-severity "no behavior surface touched -- no -coverage finding" line than to skip it and leave the CEO without -evidence to weigh. (Earlier revisions of this skill paired test-coverage -with auth and doc-writer as conditional for symmetry; that symmetry -broke when test evidence became load-bearing.) - -The test-coverage-expert is paired with the devx-ux-expert lens and -defends the user-promise contracts the DevX persona enumerates (CLI -surface, error wording, install idempotency, lockfile determinism, auth -resolution). It MUST verify "no test exists" claims with `view`/`grep` -on the test tree before emitting a finding -- false-positive coverage -findings destroy trust in the field. It does NOT compute coverage -percentages, does NOT flag tests for pure refactors, and does NOT -duplicate python-architect on test-code design. - -## Routing matrix (CEO synthesis emphasis only) - -These routes describe WHICH specialist's findings the CEO weights more -heavily for a given PR type. They do NOT change which personas run -- -every mandatory persona always runs. Routing is a CEO synthesis hint. - -- **Architecture-heavy PR** -> CEO weights Python Architect on - abstraction calls; CLI Logging on consistency. -- **CLI UX PR** -> CEO weights DevX UX on command surface; CLI Logging - on output paths; Growth Hacker on first-run conversion. -- **Security PR** -> CEO biases toward Supply Chain Security on default - behavior; DevX UX flags ergonomics regression from any mitigation. -- **Auth PR** (auth-expert active) -> CEO weights Auth Expert on - AuthResolver / token precedence; Supply Chain on token-scoping. -- **Docs / release / comms PR** (doc-writer active) -> CEO weights Doc - Writer on accuracy and voice; Growth Hacker on hook and story angle. -- **Behavior-change PR** (test-coverage active) -> CEO weights Test - Coverage Expert on regression-trap presence; DevX UX on which user - promises the change touches. A blocking-severity coverage finding on - a critical-promise surface (auth, lockfile, install, marketplace, - hooks) is the highest signal in this routing. -- **Full panel** (default) -> CEO synthesizes equally; calls out any - dissent in `dissent_notes`. - -## Execution checklist - -Work through these steps in order. Do not skip ahead. Do not emit any -output to the PR before step 6. - -1. **Read PR context** (the orchestrating workflow already fetched it - via `gh pr view` / `gh pr diff`). Identify changed files for the - conditional panelist routing decisions (auth-expert and doc-writer). - -2. **Resolve the conditional panelists** using the rules above. Decide - for EACH conditional persona: spawn active OR spawn with - `active: false` + an `inactive_reason`. Either way, all three - conditional personas ARE spawned -- the schema requires uniform - return shape. - -3. **Fan out panelist tasks.** Spawn the following tasks in PARALLEL - via the `task` tool, one task per persona: - - `python-architect` (also asked to supply `extras.diagrams`: - `class_diagram` (mermaid `classDiagram`), `component` (mermaid - `flowchart TD`), and OPTIONAL `sequence` (mermaid - `sequenceDiagram`) blocks per the persona's section 1/2/3 contract) - - `cli-logging-expert` - - `devx-ux-expert` - - `supply-chain-security-expert` - - `oss-growth-hacker` - - `auth-expert` (always - active per step 2) - - `doc-writer` (always - active per step 2) - - `test-coverage-expert` (always - active per step 2) - - Each task prompt MUST: - - Reference its persona file by relative path so the subagent loads - its own scope, lens, and anti-patterns. - - Include the PR number, title, body, and diff (passed inline). - - Cite `assets/panelist-return-schema.json` and require the subagent - to emit JSON matching that schema as its FINAL message. - - State the calibrated severity contract: "Use `severity: blocking` - ONLY for correctness regressions, security/auth bypasses, or - architectural faults that compound, with explicit rationale. - Default substantive feedback to `recommended`. Use `nit` for - one-line polish. The panel is advisory; nothing you return blocks - merge -- pick the severity that honestly matches your signal - strength." - - Restate the output contract: NO `gh` write commands, NO posting - comments, NO label changes, NO touching PR state. JSON return only. - -4. **S4 schema gate.** When each panelist task returns, parse the JSON - and validate against `assets/panelist-return-schema.json`. On - validation failure: - - Re-spawn that ONE panelist with an explicit error message pointing - at the violated rule. - - Maximum two re-spawn attempts per panelist. If still malformed, - synthesize a placeholder - `{persona: "", active: true, summary: "Schema failure -- see - extras.", findings: [], extras: {schema_failure: ""}}` - and surface the failure in the CEO arbitration prompt. - -5. **Spawn the CEO synthesizer task.** Pass the full set of validated - panelist JSON returns to a `task` invocation that loads - `../../agents/apm-ceo.agent.md`. The prompt MUST: - - Provide all panelist returns as structured input. - - Ask for: headline, arbitration prose, principle alignment (only - applicable principles), curated recommended_followups (prioritized - by signal, NOT a re-listing of every finding), ship_recommendation - (stance + prose). - - Cite `assets/ceo-return-schema.json` and require JSON return. - - Restate the contract: the panel is advisory. The CEO does NOT pick - a verdict label. The `ship_recommendation.stance` is prose for the - human reviewer, not a gate. NO `gh` write commands. - - Validate the CEO return against `assets/ceo-return-schema.json`. On - failure, re-spawn once with the violation cited. - -6. **Resolve the notification audience.** The advisory comment must - surface in the inboxes of the people who will act on it. Run: - - ``` - gh pr view --json author,reviewRequests - ``` - - Build `notify_audience` as the deduplicated list: - - the PR author's `@login` (always included); - - every requested reviewer's `@login` (these are the - CODEOWNERS-resolved reviewers GitHub auto-requested for the - touched paths, plus any explicitly-requested human reviewers); - - every requested team's `@org/team-slug` (CODEOWNERS team - entries). - - Filter out: - - bot logins (login ending in `[bot]` or matching - `dependabot|github-actions|copilot-pull-request-reviewer`); - - the orchestrator's own identity (avoid self-ping). - - Cap the final list at 6 handles to avoid notification noise (PR - author + up to 5 reviewers/teams). If the cap trims, prefer team - handles over individual logins. Pass the resulting list to the - template renderer as `notify_audience`. - - This step replaces the maintainer-notification signal that the - pre-advisory verdict labels carried. It is the only mechanism by - which a fresh panel pass announces itself. - -7. **Render the comment.** Load `assets/recommendation-template.md`, - fill the placeholders from the panelist + CEO JSON, and emit it as - exactly ONE comment. - - Filling rules: - - The per-persona summary table renders ONLY active panelists, one - row per persona, with finding counts by severity and the persona's - `summary` field. - - The mermaid diagrams come from `python-architect.extras.diagrams`. - If absent, render the placeholder lines from the template (do NOT - invent diagrams). - - The recommended follow-ups list renders the CEO's curated subset, - not every finding. Full per-persona findings collapse at the bottom. - - NEVER render the words "Verdict", "APPROVE", "REJECT", "blocked", - "merge gate", or any equivalent. The panel is advisory. - -8. **Sweep labels** via `safe-outputs.remove-labels`. The list MUST be - `[panel-review, panel-approved, panel-rejected]` -- always all three, - regardless of which are currently on the PR. `panel-review` is the - re-run idempotency reset; the other two are LEGACY VERDICT LABELS - from the pre-advisory regime that have no meaning under the advisory - contract and would mislead readers if left on a freshly-reviewed PR. - `safe-outputs.remove-labels` is idempotent on missing labels, so - sweeping all three on every run is safe and self-healing. NO - verdict labels are applied. - -## Output contract (non-negotiable) - -- Exactly ONE comment per panel run, rendered from - `assets/recommendation-template.md`. The `safe-outputs.add-comment.max: - 2` is a fail-soft ceiling; the discipline lives here. -- Exactly ONE `remove-labels` call sweeping - `[panel-review, panel-approved, panel-rejected]`. -- NO `add-labels` call. The advisory regime has no verdict to encode. -- Subagents (panelists + CEO) NEVER write to PR state, NEVER call `gh - pr comment`, NEVER call `gh pr edit --add-label`. They return JSON. - The orchestrator is the sole writer. -- Never invent new top-level template sections or drop existing ones. - -## Gotchas - -- **Roster invariant.** The frontmatter description, the roster table, - the conditional rules, the recommendation template, and the JSON - schema MUST agree on the persona set. If you change one, change all - in the same edit. -- **Calibrated severity discipline.** The advisory regime relies on - panelists honestly distinguishing `blocking` from `recommended`. If a - panelist marks everything `blocking`, the comment becomes noisy and - the maintainer learns to ignore the field. The panelist prompts state - the contract explicitly; the CEO arbitration prose is the safety - valve when a panelist over-flags. -- **Mermaid diagrams are template-required.** The python-architect - persona is asked to supply `extras.diagrams.class_diagram`, - `extras.diagrams.component`, and the OPTIONAL - `extras.diagrams.sequence`. The template renders nothing when they - are missing -- it does NOT invent diagrams. Real diagrams are - what makes the comment scannable for the human reviewer. -- **Mermaid `classDiagram` `:::cssClass` shorthand gotcha.** GitHub's - mermaid renderer rejects `:::cssClass` appended to relationship - lines (e.g. `A *-- B:::touched`); use standalone - `class Name:::cssClass` declarations instead. Authority: - `python-architect.agent.md:146-154`. -- **Doc-writer detects DRIFT, not just edits.** When the PR changes - user-facing code that SHOULD have updated docs but did not, doc-writer - surfaces that as a finding. The conditional rule above is necessary - but not sufficient -- doc-writer reasons about doc consistency given - the diff, not just whether doc files were touched. -- **False-negative auth gotcha.** Auth regressions can be introduced - from non-auth files that change the inputs to auth -- host - classification, dependency parsing, clone URL construction, HTTP - authorization headers, or call sites that bypass `AuthResolver`. If - a diff changes how a remote host, org, token source, or fallback path - is selected and you are not certain it is auth-neutral, activate - auth-expert as `active: true`. -- **Test-coverage probe is mandatory.** The test-coverage-expert MUST - verify "no test exists for X" via `view`/`grep` on the `tests/` tree - before emitting a finding. A false-positive coverage finding (test - exists but persona claimed it does not) destroys maintainer trust in - the field. The persona scope file enforces this; the orchestrator - passes the diff and trusts the persona to probe. -- **Subagent write enforcement is contract-based, not sandbox-based.** - Tool permissions are workflow-scoped, not subagent-scoped, so every - spawned task technically inherits the same `gh` toolset. The - "subagents must not write" rule is enforced by the prompt contract in - each `.agent.md` plus the `safe-outputs.add-comment.max: 2` - fail-soft. If a subagent ever tries to post a comment, the cap - catches it. -- **No verdict-label reset workflow.** The previous regime had a - companion workflow `pr-panel-label-reset.yml` that stripped verdict - labels on every push. The advisory regime has no verdict labels to - strip; that workflow is removed. diff --git a/.agents/skills/apm-review-panel/assets/ceo-return-schema.json b/.agents/skills/apm-review-panel/assets/ceo-return-schema.json deleted file mode 100644 index f4019391..00000000 --- a/.agents/skills/apm-review-panel/assets/ceo-return-schema.json +++ /dev/null @@ -1,78 +0,0 @@ -{ - "$schema": "http://json-schema.org/draft-07/schema#", - "$id": "ceo-return-schema.json", - "title": "APM Review Panel - CEO Synthesizer Return Shape (advisory regime)", - "description": "Shape the apm-ceo synthesizer MUST return when invoked by the apm-review-panel skill. The CEO produces ARBITRATION and SHIP-RECOMMENDATION prose. The CEO does NOT pick a binary verdict; the panel is advisory. The orchestrator renders these fields into the comment template.", - "type": "object", - "required": ["headline", "arbitration", "principle_alignment", "ship_recommendation"], - "additionalProperties": false, - "properties": { - "headline": { - "type": "string", - "description": "One-sentence framing of what this PR does and why it matters. Top of the comment. Under 240 chars; ASCII only." - }, - "arbitration": { - "type": "string", - "description": "One to three paragraphs of strategic framing: what the PR unlocks, how the panel signals converge, dissent (if any) and how to weigh it. Plain markdown." - }, - "dissent_notes": { - "type": "string", - "description": "Optional. One or two sentences naming explicit dissent between panelists and how the CEO weighs it. Empty string if all panelists agree." - }, - "principle_alignment": { - "type": "object", - "description": "Per-APM-principle one-liner. Skip principles that don't apply (omit the key). Keys are stable; values are short prose.", - "additionalProperties": false, - "properties": { - "portable_by_manifest": { "type": "string" }, - "secure_by_default": { "type": "string" }, - "governed_by_policy": { "type": "string" }, - "multi_harness_multi_host": { "type": "string" }, - "oss_community_driven": { "type": "string" }, - "pragmatic_as_npm": { "type": "string" } - } - }, - "growth_amplification": { - "type": "string", - "description": "Optional. One short paragraph on adoption/contributor signal worth amplifying (drawn from oss-growth-hacker's findings or extras.growth_strategy_note). Empty string if not applicable." - }, - "recommended_followups": { - "type": "array", - "description": "Ordered list of post-merge or in-PR follow-ups, highest signal first. Each item names the originating persona and why it matters. NONE of these block merge. Empty list is valid (nothing to follow up on).", - "items": { - "type": "object", - "required": ["from_persona", "summary", "why"], - "additionalProperties": false, - "properties": { - "from_persona": { - "type": "string", - "description": "The panelist whose finding this follow-up condenses." - }, - "summary": { "type": "string" }, - "why": { "type": "string" }, - "blocking": { - "type": "boolean", - "description": "Optional. Set true ONLY if the underlying finding was severity=blocking. The orchestrator still does not gate; this only affects rendering emphasis.", - "default": false - } - } - } - }, - "ship_recommendation": { - "type": "object", - "required": ["stance", "prose"], - "additionalProperties": false, - "properties": { - "stance": { - "type": "string", - "enum": ["ship_now", "ship_with_followups", "needs_discussion", "needs_rework"], - "description": "Advisory stance. NEVER auto-applied as a label or verdict. The maintainer reads this as one input among many." - }, - "prose": { - "type": "string", - "description": "One short paragraph naming the recommended next action and the highest-signal follow-up to track. Plain markdown." - } - } - } - } -} diff --git a/.agents/skills/apm-review-panel/assets/panelist-return-schema.json b/.agents/skills/apm-review-panel/assets/panelist-return-schema.json deleted file mode 100644 index cffb3cec..00000000 --- a/.agents/skills/apm-review-panel/assets/panelist-return-schema.json +++ /dev/null @@ -1,153 +0,0 @@ -{ - "$schema": "http://json-schema.org/draft-07/schema#", - "$id": "panelist-return-schema.json", - "title": "APM Review Panel - Panelist Return Shape (advisory regime)", - "description": "Shape every panel persona MUST return when invoked by the apm-review-panel skill. Single findings array with explicit severity. There is NO binary verdict; the orchestrator does not compute APPROVE/REJECT. Severity expresses the panelist's signal strength; the CEO synthesizer arbitrates and the human reviewer ships.", - "type": "object", - "required": ["persona", "active", "findings", "summary"], - "additionalProperties": false, - "properties": { - "persona": { - "type": "string", - "description": "Persona slug. MUST equal the .agent.md filename stem.", - "enum": [ - "python-architect", - "cli-logging-expert", - "devx-ux-expert", - "supply-chain-security-expert", - "oss-growth-hacker", - "auth-expert", - "doc-writer", - "test-coverage-expert" - ] - }, - "active": { - "type": "boolean", - "description": "Set to false ONLY for conditional personas (auth-expert, doc-writer, test-coverage-expert) when their fast-path file triggers and fallback self-check both miss. All mandatory personas MUST set active=true. When false, findings MUST be empty and inactive_reason MUST be a one-sentence explanation citing the touched files." - }, - "inactive_reason": { - "type": "string", - "description": "Required when active=false. One sentence citing the touched files." - }, - "summary": { - "type": "string", - "description": "One-line takeaway from this persona, suitable for the per-persona summary table in the comment. Imperative or declarative; under 200 chars; ASCII only. Required even when findings is empty (e.g. 'No persona-relevant surface touched; ship.')." - }, - "findings": { - "type": "array", - "description": "Findings raised by this persona, ordered by signal strength (most important first). Empty array means 'no concerns from this lens'. The ADVISORY regime uses three severities; orchestrator does NOT gate on any of them.", - "items": { "$ref": "#/definitions/finding" } - }, - "extras": { - "type": "object", - "description": "Persona-specific structured payload. Reserved for python-architect mermaid diagrams (class_diagram + component + optional sequence) and oss-growth-hacker side-channel notes. Never affects shipping.", - "additionalProperties": true, - "properties": { - "diagrams": { - "type": "object", - "description": "Reserved for python-architect. Mermaid blocks (string each) used by the comment template. If absent, the orchestrator renders a placeholder.", - "properties": { - "class_diagram": { - "type": "string", - "description": "Mermaid classDiagram of the problem-space classes the PR participates in, with pattern stereotypes annotated. Required by python-architect.agent.md section 1." - }, - "component": { "type": "string", "description": "Mermaid flowchart of the components/data flow touched by the PR." }, - "sequence": { "type": "string", "description": "Mermaid sequenceDiagram of the user-visible behavior change." } - } - }, - "growth_strategy_note": { - "type": "string", - "description": "Reserved for oss-growth-hacker. Side-channel note for WIP/growth-strategy.md." - } - } - } - }, - "definitions": { - "finding": { - "type": "object", - "required": ["severity", "summary", "rationale"], - "additionalProperties": false, - "properties": { - "severity": { - "type": "string", - "enum": ["blocking", "recommended", "nit"], - "description": "blocking: a correctness regression, security/auth bypass, or architectural fault that compounds. Use SPARINGLY and only with explicit evidence; the comment surfaces these prominently but does NOT block merge. recommended: a worthwhile improvement that the author and maintainer should weigh; the default for substantive feedback. nit: one-line polish; trivially skippable." - }, - "summary": { - "type": "string", - "description": "One-line description of the finding. ASCII only. No emojis." - }, - "rationale": { - "type": "string", - "description": "WHY this matters. Cite the rule, pattern, or architectural invariant violated. ASCII only." - }, - "file": { - "type": "string", - "description": "Optional repo-relative path to the file." - }, - "line": { - "type": "integer", - "description": "Optional line number." - }, - "suggestion": { - "type": "string", - "description": "Optional concrete fix (diff hint, replacement code, command to run)." - }, - "evidence": { - "type": "object", - "description": "Optional irrefutable evidence backing this finding -- a real test that exists (passed/failed) or a real test that should exist but does not (missing). When present, the apm-ceo synthesizer treats this as LOAD-BEARING: a passed test proves the asserted user promise holds AT THE STATED TIER; a failed test proves it does not; a missing test on a critical-promise surface proves the regression-trap gap. Tier matters: a `unit` passed does NOT certify the promise at integration-with-fixtures or e2e tier, and a critical user-promise surface (CLI command, install pipeline, lockfile, auth, hooks, marketplace, cross-module integration -- see test-coverage-expert tier-floor matrix) requires `integration-with-fixtures` or `e2e` to fully certify. The CEO does not arbitrate against a passed/failed outcome except by naming a specific reason (test was wrong, tier was below the surface floor, environment issue, flakiness with run-count). REQUIRED on every test-coverage-expert finding (the persona's contract); STRONGLY ENCOURAGED on any finding from any other persona that points at a test (e.g. supply-chain-security citing a test proving an exploit, devx-ux citing a test proving an error wording).", - "required": ["outcome", "tier"], - "additionalProperties": false, - "properties": { - "test_file": { - "type": "string", - "description": "Repo-relative path to the test file. Required when outcome is passed/failed; for outcome=missing this is the path where the test SHOULD live." - }, - "test_name": { - "type": "string", - "description": "Optional fully-qualified test identifier (e.g. 'TestFoo::test_bar' or 'tests/file.py::test_baz'). Lets a maintainer click straight to the assertion." - }, - "outcome": { - "type": "string", - "enum": ["passed", "failed", "missing", "manual", "unknown"], - "description": "passed: the test exists in the diff or in main and proves the user promise AT THE STATED TIER on this PR's commit. failed: the test exists and does NOT pass on this PR's commit (CEO weighs heavily; this is the load-bearing case for blocking). missing: no test exists at the expected location AT THE REQUIRED TIER for the surface; the persona has probed via view/grep and confirmed absence. Use `outcome=missing` with the floor tier when only sub-floor evidence exists for a critical surface (e.g. unit tests exist but the install-pipeline surface needs integration-with-fixtures). manual: only manual verification; counts as no automated guardrail. unknown: outcome was not verifiable in this run; persona MUST explain why in the rationale." - }, - "tier": { - "type": "string", - "enum": ["unit", "integration-with-fixtures", "e2e", "manual-only", "static"], - "description": "Tier of evidence. unit: function-level test with mocks at the boundary; cheap, fast, narrow. integration-with-fixtures: real I/O against real fixtures (real files, real subprocess, real network when tagged), no mocked surface for the asserted contract. e2e: full CLI invocation end-to-end, real artifacts, real exit codes. manual-only: only a manual procedure; no automated guardrail. static: lint / type-check / schema validation only. The CEO weights tier against the SURFACE FLOOR named in the test-coverage-expert tier-floor matrix: a `unit` passed evidence on a critical-promise surface (CLI / install / lockfile / auth / hooks / marketplace / cross-module) does NOT silence an opinion-finding from another panelist asking for `integration-with-fixtures` coverage. Required on every evidence block." - }, - "run_evidence": { - "type": "string", - "description": "Optional. For `outcome=passed` at `tier=integration-with-fixtures` or `e2e` on a critical-promise surface, the persona SHOULD have actually run the test (not just read it) and recorded the pytest invocation + pass/fail line + duration here. Verbatim, under 240 chars. Reading code is not running code (S7 DETERMINISTIC TOOL BRIDGE: facts-that-must-be-true do not survive as LLM assertions)." - }, - "assertion_excerpt": { - "type": "string", - "description": "Verbatim line(s) from the test that carry the assertion (or the line(s) that WOULD assert if outcome=missing). Keep under 240 chars; longer excerpts go in the rationale. Lets the CEO reproduce the proof without opening the file." - }, - "proves": { - "type": "string", - "description": "One sentence in USER words naming the promise the test proves (or fails to prove if missing). MUST mirror a Scenario Evidence row from the PR body when one exists -- the rubric (.github/skills/pr-description-skill/assets/scenario-evidence-rubric.md) is the shared vocabulary." - }, - "principles": { - "type": "array", - "description": "APM principle taxonomy from the scenario-evidence rubric (Portability by manifest, Secure by default, Governed by policy, Multi-harness support, Vendor-neutral, DevX, OSS). Ranks the criticality of the proven (or missing) promise.", - "items": { - "type": "string", - "enum": [ - "portability-by-manifest", - "secure-by-default", - "governed-by-policy", - "multi-harness-support", - "vendor-neutral", - "devx", - "oss" - ] - } - } - } - } - } - } - } -} diff --git a/.agents/skills/apm-review-panel/assets/recommendation-template.md b/.agents/skills/apm-review-panel/assets/recommendation-template.md deleted file mode 100644 index d28efe2b..00000000 --- a/.agents/skills/apm-review-panel/assets/recommendation-template.md +++ /dev/null @@ -1,142 +0,0 @@ - - -## APM Review Panel: `{{ ceo.ship_recommendation.stance }}` - -> {{ ceo.headline }} - -{{#if notify_audience }} -cc {{ notify_audience | space_join }} -- a fresh advisory pass is ready for your review. -{{/if}} - -{{ ceo.arbitration }} - -{{#if ceo.dissent_notes }} -**Dissent.** {{ ceo.dissent_notes }} -{{/if}} - -{{#if has_any_principle_alignment }} -**Aligned with:** {{ ceo.principle_alignment | inline_humanize_join }} -{{/if}} - -{{#if ceo.growth_amplification }} -**Growth signal.** {{ ceo.growth_amplification }} -{{/if}} - -### Panel summary - -| Persona | B | R | N | Takeaway | -|---|---|---|---|---| -{{#each active_panelists }} -| {{ persona | humanize }} | {{ count_blocking }} | {{ count_recommended }} | {{ count_nits }} | {{ summary }} | -{{/each}} - -> B = blocking-severity findings, R = recommended, N = nits. -> Counts are signal strength, not gates. The maintainer ships. - -{{#if ceo.recommended_followups.length }} -### Top {{ min(5, ceo.recommended_followups.length) }} follow-ups - -{{#each ceo.recommended_followups[:5] }} -{{ @index_plus_1 }}. **[{{ from_persona | humanize }}]{{#if blocking }} *(blocking-severity)*{{/if}}** {{ summary }} -- {{ why }} -{{/each}} -{{/if}} - -{{#if (or python_architect.extras.diagrams.class_diagram python_architect.extras.diagrams.component) }} -### Architecture - -{{#if python_architect.extras.diagrams.class_diagram }} -```mermaid -{{ python_architect.extras.diagrams.class_diagram }} -``` -{{/if}} - -{{#if python_architect.extras.diagrams.component }} -```mermaid -{{ python_architect.extras.diagrams.component }} -``` -{{/if}} - -{{#if python_architect.extras.diagrams.sequence }} -```mermaid -{{ python_architect.extras.diagrams.sequence }} -``` -{{/if}} -{{/if}} - -### Recommendation - -{{ ceo.ship_recommendation.prose }} - ---- - -
-Full per-persona findings - -{{#each panelists_in_canonical_order }} -#### {{ persona | humanize }}{{#unless active }} -- inactive{{/unless}} - -{{#if active }} -{{#if findings.length }} -{{#each findings }} -- **[{{ severity }}]** {{ summary }}{{#if file }} at `{{ file }}{{#if line }}:{{ line }}{{/if}}`{{/if}} - {{ rationale }} - {{#if suggestion }} - *Suggested:* {{ suggestion }} - {{/if}} - {{#if evidence }} - *Proof ({{ evidence.outcome }}{{#if (eq evidence.outcome "missing") }} at{{/if}}):* `{{ evidence.test_file }}{{#if evidence.test_name }}::{{ evidence.test_name }}{{/if}}`{{#if evidence.proves }} -- proves: {{ evidence.proves }}{{/if}}{{#if evidence.principles }} [{{ join evidence.principles "," }}]{{/if}} - {{#if evidence.assertion_excerpt }} - `{{ evidence.assertion_excerpt | one_line | truncate 200 }}` - {{/if}} - {{/if}} -{{/each}} -{{else}} -No findings. -{{/if}} -{{else}} -{{ inactive_reason }} -{{/if}} - -{{/each}} -
- -This panel is advisory. It does not block merge. Re-apply the -`panel-review` label after addressing feedback to re-run. diff --git a/.agents/skills/apm-review-panel/evals/README.md b/.agents/skills/apm-review-panel/evals/README.md deleted file mode 100644 index 06f6963d..00000000 --- a/.agents/skills/apm-review-panel/evals/README.md +++ /dev/null @@ -1,64 +0,0 @@ -# apm-review-panel evals - -Two complementary evals live here. - -## 1. `render_eval.py` (content / output-shape eval) - -Renders fixture JSON against the rendering rules of -`assets/recommendation-template.md`. The script is a SPECIFICATION -TEST -- it implements the same rendering rules a panel orchestrator -LLM applies in production, so we can eyeball the output offline -without spending a panel run. - -Run: - -```bash -python3 render_eval.py -``` - -Outputs `.rendered.md` next to each fixture in `fixtures/` -and prints a summary line per scenario including ASCII-only lint -(per repo encoding rule). - -### Fixtures - -- `01-ship-now-pr1084-shape.json` -- PR #1084 shape: surgical - bug-fix, all panelists APPROVE with at most polish nits, CEO - recommends `ship_now`. Verifies the COMMON case (most PRs) is - short, scannable, and doesn't bury the lede. -- `02-needs-rework-shape.json` -- PR with two correctness - regressions (path-traversal + Windows-encoding) + an architecture - smell. CEO recommends `needs_rework` with explicit blocking- - severity tags on the top follow-ups. Verifies the panel can be - HONEST about high-signal feedback without reverting to a binary - gate. - -### What "passing" looks like - -A maintainer scanning the rendered output for ~30 seconds gets: -- the stance pill (top of comment), -- the headline + 2-4 paragraph CEO synthesis, -- the per-persona summary table (one row each), -- the top-N curated follow-ups, -- and, where supplied, the architecture diagrams. - -Full per-persona findings live inside `
`. Open them when -you want depth, ignore them when you don't. - -### Adding a fixture - -Drop `--shape.json` into `fixtures/`. Schema follows -`assets/panelist-return-schema.json` (under `panelists[]`) and -`assets/ceo-return-schema.json` (under `ceo`). Re-run -`python3 render_eval.py` and inspect the new `.rendered.md`. - -## 2. `trigger-evals.json` (dispatch description eval) - -8 should-trigger + 8 should-NOT-trigger queries split 60/40 -train/val. The validation split is the ship gate per the genesis -MODULE ENTRYPOINT spec: rate >= 0.5 on should-trigger AND < 0.5 on -should-NOT-trigger. - -This is a manual eval against the dispatch description in -`SKILL.md`'s frontmatter -- run by reading the description as if -you were the harness's dispatcher LLM and classifying each query. diff --git a/.agents/skills/apm-review-panel/evals/fixtures/01-ship-now-pr1084-shape.json b/.agents/skills/apm-review-panel/evals/fixtures/01-ship-now-pr1084-shape.json deleted file mode 100644 index 499ff6ab..00000000 --- a/.agents/skills/apm-review-panel/evals/fixtures/01-ship-now-pr1084-shape.json +++ /dev/null @@ -1,114 +0,0 @@ -{ - "scenario": "ship_now", - "description": "PR #1084 shape: surgical bug-fix from external contributor, all panelists APPROVE with at most polish-level nits. CEO recommends ship_now.", - "panelists": [ - { - "persona": "python-architect", - "active": true, - "summary": "Surgical 7-line change in pipeline.py; well-scoped, no architectural debt.", - "findings": [ - {"severity": "nit", "summary": "Hoist env-var tuple to module-level constant", "rationale": "Tuple represents the named concept 'credential-helper-blocking env vars' and may be referenced as auth handling evolves.", "file": "src/apm_cli/install/pipeline.py", "line": 90} - ], - "extras": { - "diagrams": { - "class_diagram": "classDiagram\n class _preflight_auth_check:::touched {\n <>\n +probe(host) bool\n }\n class Dep {\n +name: str\n +source: str\n +is_azure_devops() bool\n }\n class AuthResolver {\n <>\n +resolve(host) Token\n }\n _preflight_auth_check ..> Dep : reads\n _preflight_auth_check ..> AuthResolver : delegates\n classDef touched fill:#fef3c7,stroke:#d97706", - "component": "flowchart TD\n A[apm install --update] --> B[_preflight_auth_check]\n B --> C{is_generic = not GitHub and not ADO}\n C -->|github.com or ADO| D[probe_env: locked down]\n C -->|GHES, GitLab, Bitbucket| E[probe_env: pop credential-blocking vars]\n D --> F[git ls-remote probe]\n E --> F", - "sequence": "sequenceDiagram\n participant U as User\n participant CLI as apm install --update\n participant PF as _preflight_auth_check\n participant Helper as git-credential-manager\n U->>CLI: install GHES dep\n CLI->>PF: probe(host=ghes.corp.example.com)\n PF->>PF: is_generic=yes; pop blocking env vars\n PF->>Helper: ls-remote with relaxed env\n Helper-->>PF: token\n PF-->>CLI: probe ok\n CLI-->>U: install proceeds" - } - } - }, - { - "persona": "cli-logging-expert", - "active": true, - "summary": "No new CLI strings, no encoding regression; failure-path UX preserved.", - "findings": [ - {"severity": "nit", "summary": "verbose param accepted but unused (pre-existing)", "rationale": "Out of scope for this PR; worth a follow-up to surface redacted probe URL on failure.", "file": "src/apm_cli/install/pipeline.py", "line": 47} - ] - }, - { - "persona": "devx-ux-expert", - "active": true, - "summary": "Pure behavioral fix that restores symmetry between install and install --update. CHANGELOG entry meets failure-mode-is-the-product bar.", - "findings": [] - }, - { - "persona": "supply-chain-security-expert", - "active": true, - "summary": "insteadOf-redirect not exploitable (probe is read-only ls-remote); host classification not spoofable; no token leak introduced.", - "findings": [ - {"severity": "nit", "summary": "Document that generic-host preflight intentionally trusts ~/.gitconfig", "rationale": "Pre-existing local-trust assumption; one-line comment helps future readers not weaken it accidentally.", "file": "src/apm_cli/install/pipeline.py", "line": 90} - ] - }, - { - "persona": "oss-growth-hacker", - "active": true, - "summary": "First external bug-fix on the GHES + credential-helper surface. Mine for a release-notes story beat.", - "findings": [ - {"severity": "recommended", "summary": "Frame next release notes around 'credential-helper support for enterprise git hosts'", "rationale": "Converts a bug-fix into a positioning signal for the exact audience this unblocks.", "file": "CHANGELOG.md", "line": 17}, - {"severity": "nit", "summary": "Capture this as a docs FAQ entry for the symptom", "rationale": "Searchable symptom should land in a troubleshooting page so future GHES adopters self-serve.", "file": "src/apm_cli/install/pipeline.py", "line": 90} - ], - "extras": { - "growth_strategy_note": "PR #1084 confirms enterprise-private-git is a real adoption surface and contributor-funnel beachhead." - } - }, - { - "persona": "auth-expert", - "active": true, - "summary": "GHES correctly classified as generic; symmetry with clone path confirmed; AuthResolver invariant intact; bearer-header injection preserved.", - "findings": [ - {"severity": "recommended", "summary": "Add regression test asserting GHES hostnames classify as generic", "rationale": "The fix's value depends on is_github_hostname returning False for non-*.ghe.com enterprise hosts; lock this contract in.", "file": "tests/unit/install/test_pipeline_auth_preflight.py", "line": 147}, - {"severity": "nit", "summary": "Could reuse dep.is_azure_devops() for ADO detection", "rationale": "Keeps host classification co-located with the dep model.", "file": "src/apm_cli/install/pipeline.py", "line": 90} - ] - }, - { - "persona": "doc-writer", - "active": false, - "summary": "No user-facing documentation surface touched (only CHANGELOG entry, which is already accurate).", - "inactive_reason": "PR touches only src/apm_cli/install/pipeline.py, tests/unit/install/test_pipeline_auth_preflight.py, and CHANGELOG.md (entry verified accurate against the diff).", - "findings": [] - }, - { - "persona": "test-coverage-expert", - "active": true, - "summary": "All four critical surfaces touched (install pipeline, auth preflight, host classification, env-var handling) have regression-trap tests in this PR; ship.", - "findings": [ - { - "severity": "recommended", - "summary": "Add a parametrized test exercising each of the three credential-helper env vars individually", - "rationale": "The current tests assert all three are popped together; a future refactor that pops two of three would still pass the existing assertion. One parametrized test per env var locks in the contract.", - "file": "tests/unit/install/test_pipeline_auth_preflight.py", - "line": 147, - "suggestion": "@pytest.mark.parametrize('env_var', ['GIT_TERMINAL_PROMPT', 'GCM_INTERACTIVE', 'GIT_ASKPASS'])", - "evidence": { - "test_file": "tests/unit/install/test_pipeline_auth_preflight.py", - "test_name": "test_install_update_does_not_disable_credential_helpers_on_generic_host", - "outcome": "passed", - "tier": "unit", - "assertion_excerpt": "assert os.environ.get('GIT_TERMINAL_PROMPT') is None", - "proves": "On non-GitHub non-ADO hosts, install --update does not block the user's system credential helpers.", - "principles": ["multi-harness-support", "vendor-neutral", "devx"] - } - } - ] - } - ], - "ceo": { - "headline": "Surgical bug-fix that unblocks GHES, GitLab, and Bitbucket users on apm install --update -- exactly the enterprise audience APM needs to win on credibility.", - "arbitration": "All seven active panelists converge: this is a 101+/1- behavioral fix that restores symmetry between `install` and `install --update` for non-GitHub, non-ADO hosts. The fix is well-scoped, well-tested (4 new unit tests covering all three env vars + ADO retention + auth-failure still raising), and the CHANGELOG entry names the failure mode in one sentence. Auth Expert verified the AuthResolver invariant is intact and bearer-header injection is preserved; Supply Chain confirmed the relaxed env on generic hosts opens no new exploit surface (probe is read-only `ls-remote`).\n\nThe most actionable signal across the panel is the Auth Expert's recommended regression test on `is_github_hostname` -- it locks in the host-classification contract this fix depends on, so a future change to that function cannot silently re-introduce #1082.", - "dissent_notes": "", - "principle_alignment": { - "multi_harness_multi_host": "Restores GHES, GitLab, and Bitbucket as fully-supported dependency hosts.", - "pragmatic_as_npm": "npm never blocks system credential helpers for registry auth; APM now matches that expectation outside its managed token path." - }, - "growth_amplification": "First external bug-fix landing the enterprise-private-git path from a real GHES user (@tillig). Worth amplifying in the next release notes as 'credential-helper support for enterprise git hosts (GHES / GitLab / Bitbucket)' and crediting the contributor.", - "recommended_followups": [ - {"from_persona": "auth-expert", "summary": "Add regression test for is_github_hostname('ghes.corp.example.com') == False", "why": "Locks in the host-classification contract this fix depends on; cheapest insurance against a silent re-regression of #1082."}, - {"from_persona": "oss-growth-hacker", "summary": "Frame the next release-notes line around 'credential-helper support for enterprise git hosts'", "why": "Converts a bug-fix into a positioning signal for the exact audience this unblocks. Credit @tillig as the first external bug-fix on this surface."}, - {"from_persona": "python-architect", "summary": "Hoist the env-var tuple to a module-level constant when a third call site appears", "why": "Pure hygiene; defer until R3 EXTRACT actually triggers (>=3 call sites)."} - ], - "ship_recommendation": { - "stance": "ship_now", - "prose": "Merge as-is. The 3 follow-ups above are non-blocking and the highest-signal one (Auth Expert's regression test) is a 5-line PR that any maintainer can land in a follow-up." - } - } -} diff --git a/.agents/skills/apm-review-panel/evals/fixtures/01-ship-now-pr1084-shape.rendered.md b/.agents/skills/apm-review-panel/evals/fixtures/01-ship-now-pr1084-shape.rendered.md deleted file mode 100644 index 740b6d0c..00000000 --- a/.agents/skills/apm-review-panel/evals/fixtures/01-ship-now-pr1084-shape.rendered.md +++ /dev/null @@ -1,137 +0,0 @@ -## APM Review Panel: `ship_now` - -> Surgical bug-fix that unblocks GHES, GitLab, and Bitbucket users on apm install --update -- exactly the enterprise audience APM needs to win on credibility. - -All seven active panelists converge: this is a 101+/1- behavioral fix that restores symmetry between `install` and `install --update` for non-GitHub, non-ADO hosts. The fix is well-scoped, well-tested (4 new unit tests covering all three env vars + ADO retention + auth-failure still raising), and the CHANGELOG entry names the failure mode in one sentence. Auth Expert verified the AuthResolver invariant is intact and bearer-header injection is preserved; Supply Chain confirmed the relaxed env on generic hosts opens no new exploit surface (probe is read-only `ls-remote`). - -The most actionable signal across the panel is the Auth Expert's recommended regression test on `is_github_hostname` -- it locks in the host-classification contract this fix depends on, so a future change to that function cannot silently re-introduce #1082. - -**Aligned with:** Multi-harness / multi-host, Pragmatic as npm - -**Growth signal.** First external bug-fix landing the enterprise-private-git path from a real GHES user (@tillig). Worth amplifying in the next release notes as 'credential-helper support for enterprise git hosts (GHES / GitLab / Bitbucket)' and crediting the contributor. - -### Panel summary - -| Persona | B | R | N | Takeaway | -|---|---|---|---|---| -| Python Architect | 0 | 0 | 1 | Surgical 7-line change in pipeline.py; well-scoped, no architectural debt. | -| CLI Logging Expert | 0 | 0 | 1 | No new CLI strings, no encoding regression; failure-path UX preserved. | -| DevX UX Expert | 0 | 0 | 0 | Pure behavioral fix that restores symmetry between install and install --update. CHANGELOG entry meets failure-mode-is-the-product bar. | -| Supply Chain Security | 0 | 0 | 1 | insteadOf-redirect not exploitable (probe is read-only ls-remote); host classification not spoofable; no token leak introduced. | -| OSS Growth Hacker | 0 | 1 | 1 | First external bug-fix on the GHES + credential-helper surface. Mine for a release-notes story beat. | -| Auth Expert | 0 | 1 | 1 | GHES correctly classified as generic; symmetry with clone path confirmed; AuthResolver invariant intact; bearer-header injection preserved. | -| Test Coverage | 0 | 1 | 0 | All four critical surfaces touched (install pipeline, auth preflight, host classification, env-var handling) have regression-trap tests in this PR; ship. | - -> B = blocking-severity findings, R = recommended, N = nits. -> Counts are signal strength, not gates. The maintainer ships. - -### Top 3 follow-ups - -1. **[Auth Expert]** Add regression test for is_github_hostname('ghes.corp.example.com') == False -- Locks in the host-classification contract this fix depends on; cheapest insurance against a silent re-regression of #1082. -2. **[OSS Growth Hacker]** Frame the next release-notes line around 'credential-helper support for enterprise git hosts' -- Converts a bug-fix into a positioning signal for the exact audience this unblocks. Credit @tillig as the first external bug-fix on this surface. -3. **[Python Architect]** Hoist the env-var tuple to a module-level constant when a third call site appears -- Pure hygiene; defer until R3 EXTRACT actually triggers (>=3 call sites). - -### Architecture - -```mermaid -classDiagram - class _preflight_auth_check:::touched { - <> - +probe(host) bool - } - class Dep { - +name: str - +source: str - +is_azure_devops() bool - } - class AuthResolver { - <> - +resolve(host) Token - } - _preflight_auth_check ..> Dep : reads - _preflight_auth_check ..> AuthResolver : delegates - classDef touched fill:#fef3c7,stroke:#d97706 -``` - -```mermaid -flowchart TD - A[apm install --update] --> B[_preflight_auth_check] - B --> C{is_generic = not GitHub and not ADO} - C -->|github.com or ADO| D[probe_env: locked down] - C -->|GHES, GitLab, Bitbucket| E[probe_env: pop credential-blocking vars] - D --> F[git ls-remote probe] - E --> F -``` - -```mermaid -sequenceDiagram - participant U as User - participant CLI as apm install --update - participant PF as _preflight_auth_check - participant Helper as git-credential-manager - U->>CLI: install GHES dep - CLI->>PF: probe(host=ghes.corp.example.com) - PF->>PF: is_generic=yes; pop blocking env vars - PF->>Helper: ls-remote with relaxed env - Helper-->>PF: token - PF-->>CLI: probe ok - CLI-->>U: install proceeds -``` - -### Recommendation - -Merge as-is. The 3 follow-ups above are non-blocking and the highest-signal one (Auth Expert's regression test) is a 5-line PR that any maintainer can land in a follow-up. - ---- - -
-Full per-persona findings - -#### Python Architect - -- **[nit]** Hoist env-var tuple to module-level constant at `src/apm_cli/install/pipeline.py:90` - Tuple represents the named concept 'credential-helper-blocking env vars' and may be referenced as auth handling evolves. - -#### CLI Logging Expert - -- **[nit]** verbose param accepted but unused (pre-existing) at `src/apm_cli/install/pipeline.py:47` - Out of scope for this PR; worth a follow-up to surface redacted probe URL on failure. - -#### DevX UX Expert - -No findings. - -#### Supply Chain Security - -- **[nit]** Document that generic-host preflight intentionally trusts ~/.gitconfig at `src/apm_cli/install/pipeline.py:90` - Pre-existing local-trust assumption; one-line comment helps future readers not weaken it accidentally. - -#### OSS Growth Hacker - -- **[recommended]** Frame next release notes around 'credential-helper support for enterprise git hosts' at `CHANGELOG.md:17` - Converts a bug-fix into a positioning signal for the exact audience this unblocks. -- **[nit]** Capture this as a docs FAQ entry for the symptom at `src/apm_cli/install/pipeline.py:90` - Searchable symptom should land in a troubleshooting page so future GHES adopters self-serve. - -#### Auth Expert - -- **[recommended]** Add regression test asserting GHES hostnames classify as generic at `tests/unit/install/test_pipeline_auth_preflight.py:147` - The fix's value depends on is_github_hostname returning False for non-*.ghe.com enterprise hosts; lock this contract in. -- **[nit]** Could reuse dep.is_azure_devops() for ADO detection at `src/apm_cli/install/pipeline.py:90` - Keeps host classification co-located with the dep model. - -#### Doc Writer -- inactive - -PR touches only src/apm_cli/install/pipeline.py, tests/unit/install/test_pipeline_auth_preflight.py, and CHANGELOG.md (entry verified accurate against the diff). - -#### Test Coverage - -- **[recommended]** Add a parametrized test exercising each of the three credential-helper env vars individually at `tests/unit/install/test_pipeline_auth_preflight.py:147` - The current tests assert all three are popped together; a future refactor that pops two of three would still pass the existing assertion. One parametrized test per env var locks in the contract. - *Suggested:* @pytest.mark.parametrize('env_var', ['GIT_TERMINAL_PROMPT', 'GCM_INTERACTIVE', 'GIT_ASKPASS']) - *Proof (test passed):* `tests/unit/install/test_pipeline_auth_preflight.py::test_install_update_does_not_disable_credential_helpers_on_generic_host` -- proves: On non-GitHub non-ADO hosts, install --update does not block the user's system credential helpers. [multi-harness-support,vendor-neutral,devx] - `assert os.environ.get('GIT_TERMINAL_PROMPT') is None` - -
- -This panel is advisory. It does not block merge. Re-apply the `panel-review` label after addressing feedback to re-run. diff --git a/.agents/skills/apm-review-panel/evals/fixtures/02-needs-rework-shape.json b/.agents/skills/apm-review-panel/evals/fixtures/02-needs-rework-shape.json deleted file mode 100644 index 67d13962..00000000 --- a/.agents/skills/apm-review-panel/evals/fixtures/02-needs-rework-shape.json +++ /dev/null @@ -1,216 +0,0 @@ -{ - "scenario": "needs_rework", - "description": "PR introduces a security regression and an architecture smell. Two panelists return blocking-severity findings; CEO recommends needs_rework but frames it as advisory.", - "panelists": [ - { - "persona": "python-architect", - "active": true, - "summary": "Refactor splits a clean module into a circular import; same logic now lives in two places.", - "findings": [ - { - "severity": "blocking", - "summary": "Circular import between resolver.py and downloader.py", - "rationale": "The new factory in resolver.py imports downloader.GithubDownloader at module top, while downloader.py imports resolver.create_resolver at module top. This works only because Python silently tolerates partial modules in sys.modules; the first import that fails will cascade across the whole install path.", - "file": "src/apm_cli/deps/resolver.py", - "line": 12, - "suggestion": "Move the factory to a third module (deps/factory.py) that both depend on, or defer the import inside the function body." - }, - { - "severity": "recommended", - "summary": "validate_url duplicated across resolver.py and downloader.py", - "rationale": "Two copies that already disagree on trailing-slash handling. R3 EXTRACT trigger fired (>=3 call sites in the diff).", - "file": "src/apm_cli/deps/resolver.py", - "line": 88 - } - ], - "extras": { - "diagrams": { - "class_diagram": "classDiagram\n class Resolver:::touched {\n +create_resolver()\n +validate_url(u) bool\n }\n class GithubDownloader:::touched {\n +download(url) Path\n +validate_url(u) bool\n }\n Resolver ..> GithubDownloader : top-level import\n GithubDownloader ..> Resolver : top-level import (cycle)\n classDef touched fill:#fef3c7,stroke:#d97706", - "component": "flowchart TD\n A[install pipeline] --> B[resolver.create_resolver]\n B --> C[downloader.GithubDownloader]\n C -->|imports at module top| B\n B -->|imports at module top| C" - } - } - }, - { - "persona": "cli-logging-expert", - "active": true, - "summary": "Two new error strings ship outside STATUS_SYMBOLS; one new emoji slipped in.", - "findings": [ - { - "severity": "blocking", - "summary": "Emoji character in error message will crash on Windows cp1252 terminals", - "rationale": "encoding.instructions.md is unambiguous: ASCII-only U+0020-U+007E. The rocket character on line 211 will raise UnicodeEncodeError under charmap.", - "file": "src/apm_cli/deps/resolver.py", - "line": 211, - "suggestion": "Replace with `[!]` or `[*]` per STATUS_SYMBOLS." - }, - { - "severity": "recommended", - "summary": "New error path bypasses _rich_error helper", - "rationale": "Direct print() loses the panel's colorization and TTY-detection. Inconsistent with the other 38 call sites in this module.", - "file": "src/apm_cli/deps/resolver.py", - "line": 203 - } - ] - }, - { - "persona": "devx-ux-expert", - "active": true, - "summary": "New error message is technically accurate but unhelpful to a user encountering it cold.", - "findings": [ - { - "severity": "recommended", - "summary": "Error message lacks an actionable next step", - "rationale": "'Invalid dependency reference' tells the user WHAT failed but not WHY or what to do. Compare to the message in install/pipeline.py:147 which suggests three remediations.", - "file": "src/apm_cli/deps/resolver.py", - "line": 211, - "suggestion": "Suffix with 'Run `apm install --verbose` to see the resolved URL, or check `apm.yml` for typos.'" - } - ] - }, - { - "persona": "supply-chain-security-expert", - "active": true, - "summary": "Path-traversal regression: new code joins user-controlled segments without validate_path_segments.", - "findings": [ - { - "severity": "blocking", - "summary": "User-controlled `dep.name` is path-joined without traversal validation", - "rationale": "path_security.instructions.md mandates validate_path_segments() at parse time for any user-provided value used in path construction. The new code at line 156 calls Path(install_dir) / dep.name directly. A malicious manifest with `name: ../../../etc/passwd` would traverse out of the install dir.", - "file": "src/apm_cli/deps/resolver.py", - "line": 156, - "suggestion": "Wrap with `validate_path_segments(dep.name, context='dep.name')` before the join, then `ensure_path_within(result, install_dir)` after." - }, - { - "severity": "recommended", - "summary": "New URL probe uses raw requests.get without timeout", - "rationale": "Default no-timeout means a hostile or hung server can stall the install pipeline indefinitely.", - "file": "src/apm_cli/deps/resolver.py", - "line": 178, - "suggestion": "Add timeout=30 (matches the convention in github_downloader.py:412)." - } - ] - }, - { - "persona": "oss-growth-hacker", - "active": true, - "summary": "No README/CHANGELOG impact; nothing to amplify or warn about externally.", - "findings": [] - }, - { - "persona": "auth-expert", - "active": false, - "summary": "Diff does not touch token resolution, AuthResolver, HostInfo, or credential paths.", - "inactive_reason": "PR touches only deps/resolver.py and deps/downloader.py refactor; no AuthResolver, HostInfo, or token-handling code in scope.", - "findings": [] - }, - { - "persona": "doc-writer", - "active": true, - "summary": "Drift: behavior change but docs/src/content/docs/reference/dependencies.md still describes the pre-refactor flow.", - "findings": [ - { - "severity": "recommended", - "summary": "docs/reference/dependencies.md describes pre-refactor flow", - "rationale": "The doc says 'resolution is performed by GithubDownloader directly' but the refactor introduces a separate Resolver. Documentation drift will mislead first-time readers.", - "file": "docs/src/content/docs/reference/dependencies.md", - "line": 47, - "suggestion": "Update the resolution-flow section to name the new Resolver, or add a note that downloader-direct resolution is being phased out." - } - ] - }, - { - "persona": "test-coverage-expert", - "active": true, - "summary": "Path-traversal regression has no regression-trap test; the malicious-name case is the test that would have caught this slip.", - "findings": [ - { - "severity": "blocking", - "summary": "No test exercises a malicious dep.name (path-traversal payload) against the new resolver join", - "rationale": "The path-traversal regression at resolver.py:156 is exactly the surface that validate_path_segments + ensure_path_within exist to defend. A test that constructs a Dep with `name='../../../etc/passwd'` and asserts the resolver raises before joining is the regression-trap that prevents this from re-shipping. Absence of such a test in tests/unit/deps/ confirmed by `grep -rn 'validate_path_segments\\|path_traversal' tests/unit/deps/` returning no match.", - "file": "tests/unit/deps/test_resolver.py", - "line": 0, - "suggestion": "Add a parametrized test with traversal payloads ('../', '..\\\\', '/etc/passwd', '..%2f..') and assert each raises ValueError before any filesystem operation.", - "evidence": { - "test_file": "tests/unit/deps/test_resolver.py", - "test_name": "test_resolver_rejects_path_traversal_in_dep_name", - "outcome": "missing", - "tier": "unit", - "assertion_excerpt": "with pytest.raises(ValueError): resolver.resolve(Dep(name='../../../etc/passwd', source='gh:...'))", - "proves": "User-controlled dep.name cannot escape the install directory via traversal payloads.", - "principles": [ - "secure-by-default", - "governed-by-policy" - ] - } - }, - { - "severity": "recommended", - "summary": "Refactor changes resolver/downloader integration but no integration test covers the cross-module flow", - "rationale": "Existing tests cover resolver.py and downloader.py in isolation; no test exercises the full install path end-to-end through both modules. A refactor that splits responsibilities across a module boundary needs at least one integration test that proves the boundary works.", - "file": "tests/integration/test_install_pipeline.py", - "line": 0, - "suggestion": "Add an integration test that installs a real (test-fixture) dependency and asserts the file ends up in the expected location after going through both Resolver and Downloader.", - "evidence": { - "test_file": "tests/integration/test_install_pipeline.py", - "test_name": "test_install_pipeline_resolver_to_downloader_e2e", - "outcome": "missing", - "tier": "integration-with-fixtures", - "assertion_excerpt": "result = install(manifest_path, scope=USER); assert (target_dir / 'expected.md').exists()", - "proves": "The new Resolver -> Downloader boundary actually delivers files to disk for a real apm.yml.", - "principles": [ - "portability-by-manifest", - "devx" - ] - } - } - ] - } - ], - "ceo": { - "headline": "Refactor direction is sound, but two correctness regressions (path traversal, Windows encoding) need to land before this can ship.", - "arbitration": "The architectural intent -- separating dependency resolution from download orchestration -- is the right call (Python Architect previously flagged the conflation as tech debt). However, this round introduces three regressions worth flagging before the next push:\n\nThe path-traversal slip at line 156 is the most important. `dep.name` is user-controlled via apm.yml, and the codebase has a strict invariant (path_security.instructions.md) that any path construction from user input MUST go through `validate_path_segments` + `ensure_path_within`. This is not a style nit -- it's the exact attack surface the centralized helpers exist to prevent.\n\nThe Windows-encoding regression is mechanically easy to fix (one character) but signals that the encoding rule is not yet automated in CI. Worth a follow-up to lint for non-ASCII bytes in source files.\n\nThe circular import is a real correctness risk Python tolerates only by accident; defer-import or extract-to-third-module both work.\n\nEverything else (validate_url duplication, error-message helpfulness, doc drift, request timeout) is recommended-severity and can land in this PR or a follow-up.", - "dissent_notes": "Python Architect and Supply Chain Security weighted the circular import as recommended vs blocking respectively; CEO sided with recommended because Python's partial-module tolerance has been stable for a decade and the import path is exercised by every install run.", - "principle_alignment": { - "secure_by_default": "The path-traversal regression is the exact failure mode `secure by default` is meant to prevent." - }, - "growth_amplification": "", - "recommended_followups": [ - { - "from_persona": "supply-chain-security-expert", - "summary": "Add validate_path_segments + ensure_path_within around the dep.name join at resolver.py:156", - "why": "User-controlled path component without traversal validation; the codebase has a hard rule.", - "blocking": true - }, - { - "from_persona": "test-coverage-expert", - "summary": "Add a regression-trap test exercising malicious dep.name (path-traversal payloads) against the resolver join", - "why": "The path-traversal slip is exactly the surface a regression-trap test would have caught; lock the contract in so this never re-ships.", - "blocking": true - }, - { - "from_persona": "cli-logging-expert", - "summary": "Replace the rocket emoji at resolver.py:211 with `[!]` per STATUS_SYMBOLS", - "why": "Will crash on Windows cp1252 terminals.", - "blocking": true - }, - { - "from_persona": "python-architect", - "summary": "Break the resolver.py <-> downloader.py circular import (factory module or deferred import)", - "why": "Tolerated by Python today but fragile to any change in import order." - }, - { - "from_persona": "doc-writer", - "summary": "Update docs/reference/dependencies.md to name the new Resolver", - "why": "Docs still describe the pre-refactor flow; drift will mislead first-time readers." - } - ], - "ship_recommendation": { - "stance": "needs_rework", - "prose": "Address the two blocking-severity items (path-traversal validation and Windows-encoding fix) before re-requesting review. The circular import is fragile but does not gate; resolve it in this PR if convenient, otherwise track as a follow-up. The remaining recommended items can land in this PR or in a series of small follow-ups -- maintainer's call." - } - }, - "notify_audience": [ - "@danielmeppiel", - "@microsoft/apm-maintainers" - ] -} diff --git a/.agents/skills/apm-review-panel/evals/fixtures/02-needs-rework-shape.rendered.md b/.agents/skills/apm-review-panel/evals/fixtures/02-needs-rework-shape.rendered.md deleted file mode 100644 index e8c2c498..00000000 --- a/.agents/skills/apm-review-panel/evals/fixtures/02-needs-rework-shape.rendered.md +++ /dev/null @@ -1,138 +0,0 @@ -## APM Review Panel: `needs_rework` - -> Refactor direction is sound, but two correctness regressions (path traversal, Windows encoding) need to land before this can ship. - -cc @danielmeppiel @microsoft/apm-maintainers -- a fresh advisory pass is ready for your review. - -The architectural intent -- separating dependency resolution from download orchestration -- is the right call (Python Architect previously flagged the conflation as tech debt). However, this round introduces three regressions worth flagging before the next push: - -The path-traversal slip at line 156 is the most important. `dep.name` is user-controlled via apm.yml, and the codebase has a strict invariant (path_security.instructions.md) that any path construction from user input MUST go through `validate_path_segments` + `ensure_path_within`. This is not a style nit -- it's the exact attack surface the centralized helpers exist to prevent. - -The Windows-encoding regression is mechanically easy to fix (one character) but signals that the encoding rule is not yet automated in CI. Worth a follow-up to lint for non-ASCII bytes in source files. - -The circular import is a real correctness risk Python tolerates only by accident; defer-import or extract-to-third-module both work. - -Everything else (validate_url duplication, error-message helpfulness, doc drift, request timeout) is recommended-severity and can land in this PR or a follow-up. - -**Dissent.** Python Architect and Supply Chain Security weighted the circular import as recommended vs blocking respectively; CEO sided with recommended because Python's partial-module tolerance has been stable for a decade and the import path is exercised by every install run. - -**Aligned with:** Secure by default - -### Panel summary - -| Persona | B | R | N | Takeaway | -|---|---|---|---|---| -| Python Architect | 1 | 1 | 0 | Refactor splits a clean module into a circular import; same logic now lives in two places. | -| CLI Logging Expert | 1 | 1 | 0 | Two new error strings ship outside STATUS_SYMBOLS; one new emoji slipped in. | -| DevX UX Expert | 0 | 1 | 0 | New error message is technically accurate but unhelpful to a user encountering it cold. | -| Supply Chain Security | 1 | 1 | 0 | Path-traversal regression: new code joins user-controlled segments without validate_path_segments. | -| OSS Growth Hacker | 0 | 0 | 0 | No README/CHANGELOG impact; nothing to amplify or warn about externally. | -| Doc Writer | 0 | 1 | 0 | Drift: behavior change but docs/src/content/docs/reference/dependencies.md still describes the pre-refactor flow. | -| Test Coverage | 1 | 1 | 0 | Path-traversal regression has no regression-trap test; the malicious-name case is the test that would have caught this slip. | - -> B = blocking-severity findings, R = recommended, N = nits. -> Counts are signal strength, not gates. The maintainer ships. - -### Top 5 follow-ups - -1. **[Supply Chain Security] *(blocking-severity)*** Add validate_path_segments + ensure_path_within around the dep.name join at resolver.py:156 -- User-controlled path component without traversal validation; the codebase has a hard rule. -2. **[Test Coverage] *(blocking-severity)*** Add a regression-trap test exercising malicious dep.name (path-traversal payloads) against the resolver join -- The path-traversal slip is exactly the surface a regression-trap test would have caught; lock the contract in so this never re-ships. -3. **[CLI Logging Expert] *(blocking-severity)*** Replace the rocket emoji at resolver.py:211 with `[!]` per STATUS_SYMBOLS -- Will crash on Windows cp1252 terminals. -4. **[Python Architect]** Break the resolver.py <-> downloader.py circular import (factory module or deferred import) -- Tolerated by Python today but fragile to any change in import order. -5. **[Doc Writer]** Update docs/reference/dependencies.md to name the new Resolver -- Docs still describe the pre-refactor flow; drift will mislead first-time readers. - -### Architecture - -```mermaid -classDiagram - class Resolver:::touched { - +create_resolver() - +validate_url(u) bool - } - class GithubDownloader:::touched { - +download(url) Path - +validate_url(u) bool - } - Resolver ..> GithubDownloader : top-level import - GithubDownloader ..> Resolver : top-level import (cycle) - classDef touched fill:#fef3c7,stroke:#d97706 -``` - -```mermaid -flowchart TD - A[install pipeline] --> B[resolver.create_resolver] - B --> C[downloader.GithubDownloader] - C -->|imports at module top| B - B -->|imports at module top| C -``` - -### Recommendation - -Address the two blocking-severity items (path-traversal validation and Windows-encoding fix) before re-requesting review. The circular import is fragile but does not gate; resolve it in this PR if convenient, otherwise track as a follow-up. The remaining recommended items can land in this PR or in a series of small follow-ups -- maintainer's call. - ---- - -
-Full per-persona findings - -#### Python Architect - -- **[blocking]** Circular import between resolver.py and downloader.py at `src/apm_cli/deps/resolver.py:12` - The new factory in resolver.py imports downloader.GithubDownloader at module top, while downloader.py imports resolver.create_resolver at module top. This works only because Python silently tolerates partial modules in sys.modules; the first import that fails will cascade across the whole install path. - *Suggested:* Move the factory to a third module (deps/factory.py) that both depend on, or defer the import inside the function body. -- **[recommended]** validate_url duplicated across resolver.py and downloader.py at `src/apm_cli/deps/resolver.py:88` - Two copies that already disagree on trailing-slash handling. R3 EXTRACT trigger fired (>=3 call sites in the diff). - -#### CLI Logging Expert - -- **[blocking]** Emoji character in error message will crash on Windows cp1252 terminals at `src/apm_cli/deps/resolver.py:211` - encoding.instructions.md is unambiguous: ASCII-only U+0020-U+007E. The rocket character on line 211 will raise UnicodeEncodeError under charmap. - *Suggested:* Replace with `[!]` or `[*]` per STATUS_SYMBOLS. -- **[recommended]** New error path bypasses _rich_error helper at `src/apm_cli/deps/resolver.py:203` - Direct print() loses the panel's colorization and TTY-detection. Inconsistent with the other 38 call sites in this module. - -#### DevX UX Expert - -- **[recommended]** Error message lacks an actionable next step at `src/apm_cli/deps/resolver.py:211` - 'Invalid dependency reference' tells the user WHAT failed but not WHY or what to do. Compare to the message in install/pipeline.py:147 which suggests three remediations. - *Suggested:* Suffix with 'Run `apm install --verbose` to see the resolved URL, or check `apm.yml` for typos.' - -#### Supply Chain Security - -- **[blocking]** User-controlled `dep.name` is path-joined without traversal validation at `src/apm_cli/deps/resolver.py:156` - path_security.instructions.md mandates validate_path_segments() at parse time for any user-provided value used in path construction. The new code at line 156 calls Path(install_dir) / dep.name directly. A malicious manifest with `name: ../../../etc/passwd` would traverse out of the install dir. - *Suggested:* Wrap with `validate_path_segments(dep.name, context='dep.name')` before the join, then `ensure_path_within(result, install_dir)` after. -- **[recommended]** New URL probe uses raw requests.get without timeout at `src/apm_cli/deps/resolver.py:178` - Default no-timeout means a hostile or hung server can stall the install pipeline indefinitely. - *Suggested:* Add timeout=30 (matches the convention in github_downloader.py:412). - -#### OSS Growth Hacker - -No findings. - -#### Auth Expert -- inactive - -PR touches only deps/resolver.py and deps/downloader.py refactor; no AuthResolver, HostInfo, or token-handling code in scope. - -#### Doc Writer - -- **[recommended]** docs/reference/dependencies.md describes pre-refactor flow at `docs/src/content/docs/reference/dependencies.md:47` - The doc says 'resolution is performed by GithubDownloader directly' but the refactor introduces a separate Resolver. Documentation drift will mislead first-time readers. - *Suggested:* Update the resolution-flow section to name the new Resolver, or add a note that downloader-direct resolution is being phased out. - -#### Test Coverage - -- **[blocking]** No test exercises a malicious dep.name (path-traversal payload) against the new resolver join at `tests/unit/deps/test_resolver.py` - The path-traversal regression at resolver.py:156 is exactly the surface that validate_path_segments + ensure_path_within exist to defend. A test that constructs a Dep with `name='../../../etc/passwd'` and asserts the resolver raises before joining is the regression-trap that prevents this from re-shipping. Absence of such a test in tests/unit/deps/ confirmed by `grep -rn 'validate_path_segments\|path_traversal' tests/unit/deps/` returning no match. - *Suggested:* Add a parametrized test with traversal payloads ('../', '..\\', '/etc/passwd', '..%2f..') and assert each raises ValueError before any filesystem operation. - *Proof (test MISSING at):* `tests/unit/deps/test_resolver.py::test_resolver_rejects_path_traversal_in_dep_name` -- proves: User-controlled dep.name cannot escape the install directory via traversal payloads. [secure-by-default,governed-by-policy] - `with pytest.raises(ValueError): resolver.resolve(Dep(name='../../../etc/passwd', source='gh:...'))` -- **[recommended]** Refactor changes resolver/downloader integration but no integration test covers the cross-module flow at `tests/integration/test_install_pipeline.py` - Existing tests cover resolver.py and downloader.py in isolation; no test exercises the full install path end-to-end through both modules. A refactor that splits responsibilities across a module boundary needs at least one integration test that proves the boundary works. - *Suggested:* Add an integration test that installs a real (test-fixture) dependency and asserts the file ends up in the expected location after going through both Resolver and Downloader. - *Proof (test MISSING at):* `tests/integration/test_install_pipeline.py::test_install_pipeline_resolver_to_downloader_e2e` -- proves: The new Resolver -> Downloader boundary actually delivers files to disk for a real apm.yml. [portability-by-manifest,devx] - `result = install(manifest_path, scope=USER); assert (target_dir / 'expected.md').exists()` - -
- -This panel is advisory. It does not block merge. Re-apply the `panel-review` label after addressing feedback to re-run. diff --git a/.agents/skills/apm-review-panel/evals/render_eval.py b/.agents/skills/apm-review-panel/evals/render_eval.py deleted file mode 100644 index 4ac664bf..00000000 --- a/.agents/skills/apm-review-panel/evals/render_eval.py +++ /dev/null @@ -1,265 +0,0 @@ -#!/usr/bin/env python3 -"""Render fixture JSON against the recommendation template's rendering rules. - -This is a SPECIFICATION TEST, not a production renderer. The orchestrator LLM -applies the same rules described in the template comment block when rendering -in production; this script lets a maintainer eyeball the output offline and -confirms the rules collapse to a compact, scannable comment. - -Usage: - python3 render_eval.py - python3 render_eval.py fixtures/01-ship-now-pr1084-shape.json -""" - -from __future__ import annotations - -import json -import sys -from pathlib import Path - -PERSONA_LABELS = { - "python-architect": "Python Architect", - "cli-logging-expert": "CLI Logging Expert", - "devx-ux-expert": "DevX UX Expert", - "supply-chain-security-expert": "Supply Chain Security", - "oss-growth-hacker": "OSS Growth Hacker", - "auth-expert": "Auth Expert", - "doc-writer": "Doc Writer", - "test-coverage-expert": "Test Coverage", -} - -PRINCIPLE_LABELS = { - "portable_by_manifest": "Portable by manifest", - "secure_by_default": "Secure by default", - "governed_by_policy": "Governed by policy", - "multi_harness_multi_host": "Multi-harness / multi-host", - "oss_community_driven": "OSS community-driven", - "pragmatic_as_npm": "Pragmatic as npm", -} - - -def humanize_persona(key: str) -> str: - return PERSONA_LABELS.get(key, key) - - -def count_findings(findings: list[dict], severity: str) -> int: - return sum(1 for f in findings if f.get("severity") == severity) - - -def render(fixture: dict) -> str: - panelists = fixture["panelists"] - ceo = fixture["ceo"] - active = [p for p in panelists if p.get("active")] - - out: list[str] = [] - - # Header: stance + headline. Top-loaded for the busy maintainer. - stance = ceo["ship_recommendation"]["stance"] - out.append(f"## APM Review Panel: `{stance}`") - out.append("") - out.append(f"> {ceo['headline']}") - out.append("") - - notify = fixture.get("notify_audience") or [] - if notify: - out.append(f"cc {' '.join(notify)} -- a fresh advisory pass is ready for your review.") - out.append("") - - out.append(ceo["arbitration"]) - out.append("") - - if ceo.get("dissent_notes"): - out.append(f"**Dissent.** {ceo['dissent_notes']}") - out.append("") - - aligned = {k: v for k, v in (ceo.get("principle_alignment") or {}).items() if v} - if aligned: - names = ", ".join(PRINCIPLE_LABELS.get(k, k) for k in aligned) - out.append(f"**Aligned with:** {names}") - out.append("") - - if ceo.get("growth_amplification"): - out.append(f"**Growth signal.** {ceo['growth_amplification']}") - out.append("") - - # Per-persona summary table. - out.append("### Panel summary") - out.append("") - out.append("| Persona | B | R | N | Takeaway |") - out.append("|---|---|---|---|---|") - for p in active: - b = count_findings(p.get("findings", []), "blocking") - r = count_findings(p.get("findings", []), "recommended") - n = count_findings(p.get("findings", []), "nit") - out.append( - f"| {humanize_persona(p['persona'])} | {b} | {r} | {n} | {p['summary']} |" - ) - out.append("") - out.append("> B = blocking-severity findings, R = recommended, N = nits.") - out.append("> Counts are signal strength, not gates. The maintainer ships.") - out.append("") - - # Top follow-ups, capped at 5. - followups = (ceo.get("recommended_followups") or [])[:5] - if followups: - n = len(followups) - out.append(f"### Top {n} follow-ups") - out.append("") - for i, f in enumerate(followups, 1): - blocking_tag = " *(blocking-severity)*" if f.get("blocking") else "" - persona = humanize_persona(f["from_persona"]) - out.append( - f"{i}. **[{persona}]{blocking_tag}** {f['summary']} -- {f['why']}" - ) - out.append("") - - # Architecture diagrams: render only when supplied. Order: class_diagram, - # component, sequence (matches python-architect.agent.md sections 1/2/3). - arch = next( - (p for p in active if p["persona"] == "python-architect"), - None, - ) - diagrams = (arch or {}).get("extras", {}).get("diagrams", {}) if arch else {} - diagram_order = ["class_diagram", "component", "sequence"] - if any(diagrams.get(k) for k in diagram_order): - out.append("### Architecture") - out.append("") - for key in diagram_order: - block = diagrams.get(key) - if not block: - continue - out.append("```mermaid") - out.append(block) - out.append("```") - out.append("") - - out.append("### Recommendation") - out.append("") - out.append(ceo["ship_recommendation"]["prose"]) - out.append("") - - out.append("---") - out.append("") - out.append("
") - out.append("Full per-persona findings") - out.append("") - - canonical_order = [ - "python-architect", - "cli-logging-expert", - "devx-ux-expert", - "supply-chain-security-expert", - "oss-growth-hacker", - "auth-expert", - "doc-writer", - "test-coverage-expert", - ] - by_key = {p["persona"]: p for p in panelists} - for key in canonical_order: - p = by_key.get(key) - if not p: - continue - if not p.get("active"): - out.append(f"#### {humanize_persona(key)} -- inactive") - out.append("") - out.append(p.get("inactive_reason", "Not in scope for this PR.")) - out.append("") - continue - out.append(f"#### {humanize_persona(key)}") - out.append("") - findings = p.get("findings", []) - if not findings: - out.append("No findings.") - out.append("") - continue - for f in findings: - loc = "" - if f.get("file"): - loc = f" at `{f['file']}" - if f.get("line"): - loc += f":{f['line']}" - loc += "`" - out.append(f"- **[{f['severity']}]** {f['summary']}{loc}") - out.append(f" {f['rationale']}") - if f.get("suggestion"): - out.append(f" *Suggested:* {f['suggestion']}") - ev = f.get("evidence") - if ev: - outcome = ev.get("outcome", "unknown") - tf = ev.get("test_file", "") - tn = ev.get("test_name", "") - ref = tf + (f"::{tn}" if tn and tf else "") if tf else (tn or "(no test ref)") - proves = ev.get("proves", "") - principles = ev.get("principles", []) or [] - tags = (" [" + ",".join(principles) + "]") if principles else "" - if outcome == "passed": - line = f" *Proof (test {outcome}):* `{ref}`" - elif outcome == "failed": - line = f" *Proof (test FAILED):* `{ref}`" - elif outcome == "missing": - line = f" *Proof (test MISSING at):* `{ref}`" - elif outcome == "manual": - line = f" *Proof (manual only):* `{ref}`" - else: - line = f" *Proof ({outcome}):* `{ref}`" - if proves: - line += f" -- proves: {proves}" - line += tags - out.append(line) - ax = ev.get("assertion_excerpt") - if ax: - ax_one = " ".join(ax.split()) - if len(ax_one) > 200: - ax_one = ax_one[:197] + "..." - out.append(f" `{ax_one}`") - out.append("") - out.append("
") - out.append("") - out.append( - "This panel is advisory. It does not block merge. Re-apply the " - "`panel-review` label after addressing feedback to re-run." - ) - return "\n".join(out) - - -def lint_ascii(text: str) -> list[str]: - """ASCII-only enforcement (encoding.instructions.md).""" - issues: list[str] = [] - for i, line in enumerate(text.splitlines(), 1): - for ch in line: - cp = ord(ch) - if ch == "\n" or ch == "\t": - continue - if cp < 0x20 or cp > 0x7E: - issues.append(f"line {i}: non-ASCII char U+{cp:04X} ({ch!r})") - break - return issues - - -def main() -> int: - here = Path(__file__).parent - if len(sys.argv) > 1: - paths = [Path(sys.argv[1])] - else: - paths = sorted((here / "fixtures").glob("*.json")) - - for path in paths: - fixture = json.loads(path.read_text()) - rendered = render(fixture) - out_path = path.with_suffix(".rendered.md") - out_path.write_text(rendered + "\n") - line_count = len(rendered.splitlines()) - char_count = len(rendered) - ascii_issues = lint_ascii(rendered) - status = "OK" if not ascii_issues else f"FAIL ({len(ascii_issues)} ASCII)" - print( - f"[{status}] {path.name} -> {out_path.name} " - f"({line_count} lines, {char_count} chars)" - ) - for issue in ascii_issues: - print(f" {issue}") - return 0 - - -if __name__ == "__main__": - raise SystemExit(main()) diff --git a/.agents/skills/apm-review-panel/evals/trigger-evals.json b/.agents/skills/apm-review-panel/evals/trigger-evals.json deleted file mode 100644 index 22289638..00000000 --- a/.agents/skills/apm-review-panel/evals/trigger-evals.json +++ /dev/null @@ -1,31 +0,0 @@ -{ - "description": "Trigger evals for the apm-review-panel skill dispatch description. 8 should-trigger + 8 should-NOT-trigger. 60/40 train/val split (10 train, 6 val). Validation split is the ship gate.", - "should_trigger": { - "train": [ - "review this PR with the expert panel", - "run the apm-review-panel against this branch", - "do a multi-persona review of PR #1234", - "panel-review this PR", - "get the architecture + security + UX review on this PR" - ], - "val": [ - "ask the python architect, cli logging expert, and security expert to weigh in on this PR", - "have the apm panel give a recommendation on this PR", - "i need a cross-cutting expert review of this change before merge" - ] - }, - "should_not_trigger": { - "train": [ - "give me a code review of this file", - "what does the python architect think of this class hierarchy", - "review this docstring", - "fix the lint errors in src/", - "what is the apm-review-panel skill" - ], - "val": [ - "explain how the auth resolver works", - "summarize the diff of PR #1234", - "draft a PR description for me" - ] - } -} diff --git a/.agents/skills/apm-strategy/SKILL.md b/.agents/skills/apm-strategy/SKILL.md deleted file mode 100644 index 69d1b82d..00000000 --- a/.agents/skills/apm-strategy/SKILL.md +++ /dev/null @@ -1,34 +0,0 @@ ---- -name: apm-strategy -description: >- - Activate for changes to project positioning, release communication, - community-facing artifacts, or breaking-change decisions in - microsoft/apm. Triggers on README, MANIFESTO, PRD, CHANGELOG, release - workflows, and issue templates. ---- - -# APM Strategy Skill - -[APM CEO persona](../../agents/apm-ceo.agent.md) - -## When to activate - -- Edits to `README.md`, `MANIFESTO.md`, `PRD.md`, `APPROACH.md` -- Edits to `CHANGELOG.md` (especially Unreleased and version sections) -- Changes to `.github/ISSUE_TEMPLATE/` or `pull_request_template.md` -- Release-pipeline workflow changes - (`.github/workflows/build-release.yml`, version bumps, tagging) -- Any breaking-change discussion (deprecations, command renames, - config schema breaks) -- Any decision flagged as "strategic" by another reviewer - -## Key rules - -- Ground every claim in `gh` CLI evidence (stars, issues, PRs, - releases, traffic, contributors). No vibes-based assertions. -- Every breaking change ships with a `CHANGELOG.md` entry and a - one-line migration note. -- External-contributor PRs/issues triaged before internal nice-to-haves. -- Position against incumbents; never name-drop them in shipped copy. -- Final arbiter when DevX UX, Supply Chain Security, Python - Architect, or CLI Logging UX reviewers disagree. diff --git a/.agents/skills/apm-triage-panel/SKILL.md b/.agents/skills/apm-triage-panel/SKILL.md deleted file mode 100644 index 8af38938..00000000 --- a/.agents/skills/apm-triage-panel/SKILL.md +++ /dev/null @@ -1,424 +0,0 @@ ---- -name: apm-triage-panel -description: >- - Use this skill to triage a single newly opened, reopened, or - `status/needs-triage`-labelled issue in microsoft/apm. Emit one - synthesized comment with a triage decision, label set, milestone, - and suggested next action. ---- - -# APM Triage Panel -- Single-Issue Triage Orchestration - -The panel is fixed at **3 mandatory specialist lenses + up to 3 -conditional lenses + 1 arbiter lens = up to 6 active persona sections -in one triage comment** (3 mandatory + 3 conditional). You play each -lens in turn from inside a single agent loop (progressive-disclosure -skill model -- no sub-agent dispatch). Routing chooses *which* lenses -execute; it never changes which headings appear in the final comment. - -This skill mirrors the `apm-review-panel` orchestration shape on -purpose. Same single-comment discipline, same completeness gate, same -persona-pass procedure -- only the personas, the rubric, and the -output template differ. - -## Agent roster - -| Agent | Persona | Always active? | -|-------|---------|----------------| -| [DevX UX Expert](../../agents/devx-ux-expert.agent.md) | User-Need Reviewer | Yes | -| [Supply Chain Security Expert](../../agents/supply-chain-security-expert.agent.md) | Risk-Surface Reviewer | Yes | -| [APM CEO](../../agents/apm-ceo.agent.md) | Triage Arbiter | Yes (always arbitrates) | -| [OSS Growth Hacker](../../agents/oss-growth-hacker.agent.md) | Contributor-Tone Reviewer | Conditional (see below) | -| [Python Architect](../../agents/python-architect.agent.md) | Architecture Reviewer | Conditional (see below) | -| [Doc Writer](../../agents/doc-writer.agent.md) | Documentation Reviewer | Conditional (see below) | - -Skipped by default: CLI Logging Expert, Auth Expert. Triage operates -on issue intent, not on diffs -- those personas are invoked downstream -by `apm-review-panel` once a PR exists. - -## Routing topology - -``` - devx-ux-expert supply-chain-security-expert - \_______________________/ - | - | <-- python-architect (conditional; design / - | architecture / new primitive / new schema) - | - | <-- doc-writer (conditional; docs work or - | user-facing change that needs new doc pages) - v - apm-ceo <---- oss-growth-hacker - (final call / arbiter) (conditional; tunes tone - when author is new) -``` - -- **Specialists raise findings independently** -- no implicit consensus. -- **CEO arbitrates** the theme, milestone, priority, and tone of the - reply. CEO has the final call on the decision rubric. -- **Growth Hacker, Python Architect, and Doc Writer are side-channels** - to the CEO when activated. They never block a specialist finding; - they feed the CEO's arbitration: - - Growth Hacker tunes the comment's tone for first-time and - low-interaction contributors. - - Python Architect flags feasibility and cross-cutting impact, and - pushes the decision toward `status/needs-design` when warranted. - - Doc Writer flags whether docs work is implied and whether the - suggested comment wording is grounded in the user vocabulary used - in the README and guides. - -## Conditional panelists - -Three personas are conditional: OSS Growth Hacker, Python Architect, -and Doc Writer. Each follows the same shape: an explicit YES/NO -activation rule plus an inactive-reason fallback. Maximum lenses in a -single triage = 6 (3 mandatory + 3 conditional). - -### OSS Growth Hacker - -Activate `oss-growth-hacker` if either rule below matches. - -1. **Fast-path author trigger.** Activate the Growth Hacker lens - immediately when the issue's author meets ANY of: - - GitHub `author_association` is `FIRST_TIME_CONTRIBUTOR`, - `FIRST_TIMER`, or `NONE` against `microsoft/apm`. - - Author has fewer than 3 prior interactions (issues + PRs + - comments) on `microsoft/apm`. - - Issue body explicitly says "first issue", "new to APM", or - similar. - -2. **Fallback self-check.** If author signals are ambiguous, answer - this before activating the lens: - - > Would the warmth, framing, or pointer-set in the reply meaningfully - > change if I knew this was someone's first interaction with the - > project? Answer YES or NO with one sentence. - > If unsure, answer YES. - -Routing rule: - -- **YES** -> take the OSS Growth Hacker lens (per the Persona pass - procedure) and capture its tone-tuning findings. -- **NO** -> record `OSS Growth Hacker inactive reason: ` - in working notes; do not take the lens. - -### Python Architect - -Activate `python-architect` if either rule below matches. - -1. **Fast-path label / scope trigger.** Activate the Architecture - Reviewer lens immediately when ANY of: - - The issue carries `type/architecture` (current or proposed) or - the `breaking-change` preserved label. - - The issue body proposes a new top-level CLI command, or a schema - change to `apm.yml`, `apm.lock.yaml`, or `apm-policy.yml`. - - The issue body contains keywords indicating cross-module or - cross-file work, a new module, a new pattern, a new contract, or - a new primitive design -- e.g. "refactor", "rearchitect", "new - module", "design", "abstraction", "schema change", "pluggable", - "introduce X pattern". - -2. **Fallback self-check.** If the issue is ambiguous, answer this - before activating the lens: - - > Does this issue, if accepted as written, require a cross-cutting - > design decision (interface, data model, migration boundary, or - > new primitive) before code can land safely? Answer YES or NO - > with one sentence. If unsure, answer YES. - -Routing rule: - -- **YES** -> take the Python Architect lens. Capture: feasibility of - the design as proposed, callouts of cross-cutting impact, and - whether the issue should land as `status/needs-design` instead of - `status/accepted`. -- **NO** -> record `Python Architect inactive reason: ` - in working notes; do not take the lens. - -### Doc Writer - -Activate `doc-writer` if either rule below matches. - -1. **Fast-path label / scope trigger.** Activate the Documentation - Reviewer lens immediately when ANY of: - - The issue is `type/docs` or carries `area/docs-site` (current or - proposed). - - The issue body proposes documentation, README, reference, guide, - or migration-note changes. - - The issue is a user-facing feature that will require new doc - pages -- e.g. a new CLI flag, a new primitive, a new authoring - concept. - -2. **Fallback self-check.** If the issue is ambiguous, answer this - before activating the lens: - - > Will an implementing PR for this issue need to add or change - > user-facing documentation in `docs/src/content/docs/` or in the - > README? Answer YES or NO with one sentence. If unsure, answer - > YES. - -Routing rule: - -- **YES** -> take the Doc Writer lens. Capture: whether docs work is - implied (and whether `area/docs-site` should be added as a - secondary `area/*` so the implementing PR is reminded), and whether - the proposed comment wording is clear and grounded in the user - vocabulary used in the README and guides. -- **NO** -> record `Doc Writer inactive reason: ` in - working notes; do not take the lens. - -## Triage decision rubric - -The CEO arbiter picks exactly ONE outcome from this rubric: - -- `accept` -- direction is clear and aligned with the README spine and - the roadmap. Assigns full label set + milestone if a current - candidate exists. -- `needs-design` -- direction is sound but the design must be settled - before code lands. Apply `status/needs-design` and name in the - comment exactly what must be designed (interface, data model, - migration, security boundary). -- `decline-with-reason` -- out of scope for APM as positioned by the - README spine. Suggest an alternative tool, a workaround, or the - upstream project. Always courteous, always concrete. -- `duplicate-of #N` -- propose the canonical issue. The orchestrator - must verify the link resolves before posting. -- `defer-later` -- accepted in principle but no current milestone. - Sits as `status/accepted` plus `theme/* + area/*` only; no - `priority/*`, no milestone. -- `auto-handle` -- automated noise such as a daily CLI-consistency - report PR or scheduled bot issue. Propose closing if the report has - zero unaddressed High findings; otherwise propose splitting into - individual issues with the right `area/*` labels and reference back - to the parent. - -## Label-set construction rules - -Triage produces a single proposed label set. The taxonomy: - -- **Mega-themes** (one of): - `theme/portability`, `theme/security`, `theme/governance`. -- **Sub-themes** (`area/*`, one or more): - `area/multi-target`, `area/marketplace`, `area/package-authoring`, - `area/distribution`, `area/mcp-config`, `area/content-security`, - `area/lockfile`, `area/mcp-trust`, `area/audit-policy`, - `area/enterprise`, `area/cli`, `area/ci-cd`, `area/testing`, - `area/docs-site`. -- **Types** (exactly one): - `type/bug`, `type/feature`, `type/docs`, `type/refactor`, - `type/architecture`, `type/automation`, `type/release`, - `type/performance`. -- **Statuses** (exactly one): - `status/needs-triage`, `status/accepted`, `status/needs-design`, - `status/blocked`, `status/in-flight`. -- **Priorities** (optional): - `priority/high`, `priority/low`. -- **Preserved** (apply when relevant): - `breaking-change`, `good first issue`, `help wanted`, - `experimental`, `panel-review`, `dx`, `agentic-workflows`, - `dependencies`. - -Construction rules: - -- Exactly one `theme/` label is required UNLESS the issue is - pure infra (only `area/cli`, `area/ci-cd`, `area/testing`, or - `area/docs-site` apply, with no product surface implication). State - this explicitly in the per-lens notes when omitting the theme. -- Multi-theme labels are allowed; the **primary theme** is listed - first and drives the milestone. -- Exactly one `type/*` label. -- Exactly one `status/*` label. The default `status/needs-triage` is - always replaced by the triage outcome (`status/accepted`, - `status/needs-design`, `status/blocked`, etc.). Do not leave - `status/needs-triage` on a triaged issue. -- `priority/*` only on `accept` with a current milestone or next - minor. Never on `defer-later`, `needs-design`, or `decline-*`. - -## Milestone assignment rules - -- **Current patch milestone** (e.g., `0.9.x`) for bug fixes and small - DX work that fits a patch release. -- **Next minor** (e.g., `0.10.0`) for `type/feature` accepted with - `priority/high`. -- **No milestone (`null`)** for `defer-later` and `needs-design`. - -The orchestrator looks up open milestones with: - -``` -gh api repos/microsoft/apm/milestones --jq '.[]|select(.state=="open")|.title' -``` - -The lowest-numbered open patch milestone is "current patch"; the -lowest-numbered open minor is "next minor". If neither exists, set -milestone to `null` and note it. - -## Quality gates - -A triage comment passes when: - -- [ ] DevX UX Expert: real user surface identified, the request maps - (or fails to map) to a concrete README-anchored capability -- [ ] Supply Chain Security Expert: P/G/S risk surfaces assessed; if - the issue touches lockfile, marketplace, MCP config, signing, - or auth, `theme/security` or `theme/governance` is on the set -- [ ] APM CEO: theme, milestone, priority, decision, and reply tone - ratified -- [ ] OSS Growth Hacker lens taken or inactive reason recorded; if - taken, tone tuned for a new or low-interaction contributor and - the reply names a concrete next step they can take -- [ ] Python Architect lens taken or inactive reason recorded; if - taken, feasibility, cross-cutting impact, and any - `status/needs-design` recommendation are captured -- [ ] Doc Writer lens taken or inactive reason recorded; if taken, - docs implication is named and any `area/docs-site` secondary - label is proposed when the implementing PR will need new pages - -## Notes - -- This skill orchestrates a panel **in your own context** -- you are - the only agent. You load each persona's `.agent.md` reference file - on demand (progressive disclosure), assume that persona's lens to - produce its findings, then move to the next persona. Do NOT spawn - sub-agents (no `task` tool dispatch) -- the panel is a sequence of - reasoning passes inside one agent loop, not a multi-agent fan-out. -- Persona detail lives in the linked `.agent.md` files. Read each - one when you switch to that persona; do not pre-load all of them. - -## Execution checklist - -When this skill is activated for an issue, work through these steps -in order, in a single agent loop. Do not skip ahead and do not emit -any output before the final step. - -1. Read the issue context (title, body, labels, author, - `author_association`, prior comments). The orchestrating workflow - already fetches this with `gh issue view --json` -- do not - re-fetch from inside the skill. -2. Resolve the **three conditional cases** -- OSS Growth Hacker, - Python Architect, Doc Writer -- using the rules in "Conditional - panelists" above. For each, record either an activation decision - or ` inactive reason: ` in working notes. -3. For each mandatory persona (plus any conditional persona that - activated), follow the **Persona pass procedure** below, one - persona at a time. Do not try to play multiple personas in a - single pass. -4. Run the **pre-arbitration completeness gate**: - - Findings exist in working notes for the 2 mandatory specialists - (DevX UX Expert, Supply Chain Security Expert). - - For EACH of OSS Growth Hacker, Python Architect, and Doc Writer: - exactly one of ` findings` or ` inactive - reason` exists (neither = incomplete; both = inconsistent - routing). - - No persona section is missing or empty. - If any check fails, redo that persona's pass and repeat the gate. - Do not proceed to step 5 until the gate passes. -5. Take the **APM CEO** lens (load - `../../agents/apm-ceo.agent.md`) and arbitrate the collected - findings into a single decision: rubric outcome, primary theme, - `area/*` set, `type/*`, `status/*`, optional `priority/*`, - milestone, and reply tone. Still in your own context. CEO - arbitration may run only after the completeness gate has passed. -6. If the rubric outcome is `duplicate-of #N`, verify the candidate - issue exists and is open with `gh issue view N --json state,title` - before committing the link. -7. Now (and only now) load `assets/triage-template.md` and fill it - in with the collected findings, decision, label set, milestone, - and proposed comment body. -8. Emit the filled template as exactly ONE comment via the workflow's - `safe-outputs.add-comment` channel. For direct (non-workflow) - invocation, return the comment text and the structured - `triage-decision` JSON tail so an orchestrator can apply labels - and post the comment without parsing prose. This is the ONLY - output emission for the entire panel run -- no per-persona - comments, no progress comments. - -### Persona pass procedure - -For each persona, run this exact procedure in your own context: - -1. Open the persona's `.agent.md` file (linked in the roster) and - read its scope, lens, anti-patterns, and required return shape. -2. From that persona's lens, review the issue title, body, labels, - author signals, and any prior comments against the scope declared - in the file. -3. Write the findings to working notes under - `: ` (or, for an inactive conditional - persona, ` inactive reason: `). -4. Drop the persona lens before moving on. Do not emit any comment - from inside a persona pass; persona findings stay in working - notes until step 7 synthesizes them. - -## Output contract - -This contract is non-negotiable -- it is the difference between a -triage that lands as one cohesive comment and one that fragments into -per-persona noise. - -- Produce **exactly one** comment per triage run. -- Use `assets/triage-template.md` as the comment body. Keep its - section headings exactly as written. Adapt the body of each - section to the issue. Do not invent new top-level sections or drop - existing ones. -- The trailing fenced ```json block named `triage-decision` is - REQUIRED. It is the machine-readable contract that downstream - automation uses to apply labels, set the milestone, and post the - reply without parsing prose. -- ASCII only inside the comment body and JSON tail. No emojis, no - Unicode dashes, no box-drawing characters. Use `[+] [!] [x] [i] [*] [>]` - if status symbols are needed. -- CEO arbitration may run only after the completeness gate passes. -- Never emit findings as separate comments, intermediate progress - comments, or "I will now invoke X" status comments. -- Load `assets/triage-template.md` **at synthesis time only** (step - 7 above) -- not at activation, not while collecting findings. - -## Anti-patterns - -- **Over-labelling.** Do not exceed 6 labels per issue across - `theme/* + area/* + type/* + status/* + priority/* + preserved/*`. - If you find yourself reaching for 7+, prune the weakest `area/*`. -- **Milestone without status.** Never assign a milestone to an issue - whose status is not `status/accepted` or `status/in-flight`. - `needs-design` and `defer-later` are explicitly milestone-free. -- **Silent decline.** Do not auto-close or `decline-with-reason` - without a courteous reason linked to the README spine, the - manifesto, or the public roadmap. Every decline names where the - user can go instead. -- **Vague needs-design.** Never apply `status/needs-design` without - naming, in the suggested comment, exactly what must be designed - (interface, data model, migration, security boundary). "We need to - think about this" is not a design-needed reason. -- **Naked `status/needs-triage` carryover.** Triage replaces the - default `status/needs-triage` label. Leaving it on a triaged issue - is a routing bug. -- **Wildcard heuristics.** Do not activate the OSS Growth Hacker on - `*new*` or `*first*` keyword matches alone -- always cross-check - `author_association` and prior interactions on `microsoft/apm`. - Same discipline for Python Architect (do not fire on the bare word - "refactor" in unrelated context -- check the issue's actual scope) - and Doc Writer (do not fire purely on the word "docs" appearing in - passing -- the issue must propose or imply a doc-surface change). - -## Gotchas - -- **Roster invariant.** The frontmatter description, the roster - table, the conditional-panelist rule, the triage template, and the - quality gates MUST agree on the persona set. If you change one, - change all of them in the same edit. -- **No new persona required.** This skill deliberately reuses - `devx-ux-expert`, `supply-chain-security-expert`, `apm-ceo`, - `oss-growth-hacker`, `python-architect`, and `doc-writer`. Do not - create a `triage-*` persona; the README spine plus the label - taxonomy plus the existing CEO arbiter are sufficient grounding. -- **Bundle layout on the runner.** When this skill runs inside an - agentic workflow, the APM bundle is unpacked under - `.github/skills/apm-triage-panel/` first, with `.apm/skills/...` - as a fallback. The asset path is the same relative to the skill - root (`assets/triage-template.md`) in both layouts -- prefer the - `.github/...` path when present. -- **No multi-persona-in-one-pass.** Each persona has its own - `.agent.md` for a reason -- read it when you take that lens, write - the findings, then drop the lens before moving on. -- **Single-emission discipline is fragile under interruption.** If - you find yourself wanting to "post a quick partial decision and - then update it", don't. Buffer in working notes; emit once. diff --git a/.agents/skills/apm-triage-panel/assets/triage-template.md b/.agents/skills/apm-triage-panel/assets/triage-template.md deleted file mode 100644 index fb27fa0d..00000000 --- a/.agents/skills/apm-triage-panel/assets/triage-template.md +++ /dev/null @@ -1,120 +0,0 @@ - - -## Triage decision - -` | `duplicate-of: #N` | `defer-later` | `auto-handle: `> - -## Proposed labels - -``` -theme/ -area/ -area/ -type/ -status/ -priority/ - -``` - -## Milestone - - - -## Suggested next action - - - -## Suggested issue comment - -```markdown - -``` - -## Per-lens notes (collapsed) - -
-DevX UX Expert -- User-Need Reviewer - - - -
- -
-Supply Chain Security Expert -- Risk-Surface Reviewer - - - -
- -
-OSS Growth Hacker -- Contributor-Tone Reviewer - -".> - -
- -
-Python Architect -- Architecture Reviewer - -".> - -
- -
-Doc Writer -- Documentation Reviewer - -".> - -
- -
-APM CEO -- Triage Arbiter - - - -
- -```json triage-decision -{ - "decision": "", - "decision_detail": "", - "theme": "", - "areas": ["area/", "area/"], - "type": "", - "status": "", - "priority": "", - "preserved_labels": [""], - "milestone": "", - "next_action": "", - "comment_markdown": "" -} -``` diff --git a/.agents/skills/auth/SKILL.md b/.agents/skills/auth/SKILL.md deleted file mode 100644 index 9ba89448..00000000 --- a/.agents/skills/auth/SKILL.md +++ /dev/null @@ -1,59 +0,0 @@ ---- -name: auth -description: > - Activate when code touches token management, credential resolution, git auth - flows, GITHUB_APM_PAT, ADO_APM_PAT, AuthResolver, HostInfo, AuthContext, or - any remote host authentication -- even if 'auth' isn't mentioned explicitly. ---- - -# Auth Skill - -[Auth expert persona](../../agents/auth-expert.agent.md) - -## When to activate - -- Any change to `src/apm_cli/core/auth.py` or `src/apm_cli/core/token_manager.py` -- Code that reads `GITHUB_APM_PAT`, `GITHUB_TOKEN`, `GH_TOKEN`, `ADO_APM_PAT` -- Code using `git ls-remote`, `git clone`, or GitHub/ADO API calls -- Error messages mentioning tokens, authentication, or credentials -- Changes to `github_downloader.py` auth paths -- Per-host or per-org token resolution logic - -## Key rule - -All auth flows MUST go through `AuthResolver`. No direct `os.getenv()` for token variables in application code. - -## Canonical reference - -The full per-org -> global -> credential-fill -> fallback resolution flow is in [`docs/src/content/docs/getting-started/authentication.md`](../../../docs/src/content/docs/getting-started/authentication.md) (mermaid flowchart). Treat it as the single source of truth; if behavior diverges, fix the diagram in the same PR. - -## Bearer-token authentication for ADO - -ADO hosts (`dev.azure.com`, `*.visualstudio.com`) resolve auth in this order: - -1. `ADO_APM_PAT` env var if set -2. AAD bearer via `az account get-access-token --resource 499b84ac-1321-427f-aa17-267ca6975798` if `az` is installed and `az account show` succeeds -3. Otherwise: auth-failed error from `build_error_context` - -`ADO_APM_PAT` is the env var name used by the auth flow. The AAD bearer source constant lives in `src/apm_cli/core/token_manager.py` as `GitHubTokenManager.ADO_BEARER_SOURCE = "AAD_BEARER_AZ_CLI"`. - -**Stale-PAT silent fallback:** if `ADO_APM_PAT` is rejected with HTTP 401, APM retries with the az bearer and emits: - -``` -[!] ADO_APM_PAT was rejected for {host} (HTTP 401); fell back to az cli bearer. -[!] Consider unsetting the stale variable. -``` - -**Verbose source line** (one per host, emitted under `--verbose`): - -``` -[i] dev.azure.com -- using bearer from az cli (source: AAD_BEARER_AZ_CLI) -[i] dev.azure.com -- token from ADO_APM_PAT -``` - -**Diagnostic cases** (`_emit_stale_pat_diagnostic` + `build_error_context` in `src/apm_cli/core/auth.py`): - -1. No PAT, no `az`: `No ADO_APM_PAT was set and az CLI is not installed.` -> install `az`, run `az login --tenant `, or set `ADO_APM_PAT`. -2. No PAT, `az` not signed in: `az CLI is installed but no active session was found.` -> run `az login --tenant ` against the tenant that owns the org, or set `ADO_APM_PAT`. -3. No PAT, wrong tenant: `az CLI returned a token but the org does not accept it (likely a tenant mismatch).` -> run `az login --tenant `, or set `ADO_APM_PAT`. -4. PAT 401, no `az` fallback: `ADO_APM_PAT was rejected (HTTP 401) and no az cli fallback was available.` -> rotate the PAT, or install `az` and run `az login --tenant `. diff --git a/.agents/skills/cli-logging-ux/SKILL.md b/.agents/skills/cli-logging-ux/SKILL.md deleted file mode 100644 index 3b07adb7..00000000 --- a/.agents/skills/cli-logging-ux/SKILL.md +++ /dev/null @@ -1,336 +0,0 @@ ---- -name: cli-logging-ux -description: > - Use this skill when editing or creating CLI output, logging, warnings, - error messages, progress indicators, or diagnostic summaries in the APM - codebase. Activate whenever code touches console helpers (_rich_success, - _rich_warning, _rich_error, _rich_info, _rich_echo), DiagnosticCollector, - STATUS_SYMBOLS, CommandLogger, or any user-facing terminal output — even - if the user doesn't mention "logging" or "UX" explicitly. ---- - -[CLI Logging UX expert persona](../../agents/cli-logging-expert.agent.md) - -# CLI Logging & Developer Experience - -## Decision framework - -Apply these three tests to every piece of user-facing output. If a message fails any test, redesign it. - -### 1. The "So What?" Test - -Every warning must answer: *what should the user do about this?* - -``` -# Fails — not actionable, user can't do anything -Sub-skill 'my-skill' from 'my-package' overwrites existing skill - -# Passes — tells the user exactly what to do -Skipping my-skill — local file exists (not managed by APM). Use 'apm install --force' to overwrite. -``` - -If the user can't act on it, it's not a warning — it's noise. Demote to `--verbose` or remove. - -### 2. The Traffic Light Rule - -Use color semantics consistently. Never use a warning color for an informational state. - -| Color | Helper | Meaning | When to use | -|-------|--------|---------|-------------| -| Green | `_rich_success()` | Success / completed | Operation finished as expected | -| Yellow | `_rich_warning()` | User action needed | Something requires user decision | -| Red | `_rich_error()` | Error / failure | Operation failed, cannot continue | -| Blue | `_rich_info()` | Informational | Status updates, progress, summaries | -| Dim | `_rich_echo(color="dim")` | Secondary detail | Verbose-mode details, grouping headers | - -### 3. The Newspaper Test - -Can the user scan output like headlines? Top-level = what happened. Details = drill down. - -``` -# Bad — warnings break the visual flow between status and summary -[checkmark] package-name -[warning] something happened -[warning] something else happened - [tree] 3 skill(s) integrated - -# Good — clean tree, diagnostics at the end -[checkmark] package-name - [tree] 3 skill(s) integrated - -── Diagnostics ── - [warning] 2 skills replaced by a different package (last installed wins) - Run with --verbose to see details -``` - -## Inline output vs deferred diagnostics - -### Use inline output for: -- Success confirmations (`_rich_success`) -- Progress updates (`_rich_info` with indented `└─` prefix) -- Errors that halt the current operation (`_rich_error`) - -### Use DiagnosticCollector for: -- Warnings that apply across multiple packages (collisions, overwrites) -- Issues the user should know about but that don't stop the operation -- Anything that would repeat N times in a loop - -```python -# Bad — inline warning repeated per file, clutters output -for file in files: - if collision: - _rich_warning(f"Skipping {file}...") - -# Good — collect during loop, render grouped summary at the end -for file in files: - if collision: - diagnostics.skip(file, package=pkg_name) - -# Later, after the loop: -if diagnostics.has_diagnostics: - diagnostics.render_summary() -``` - -DiagnosticCollector categories: `skip()` for collisions, `overwrite()` for cross-package replacements, `warn()` for general warnings, `error()` for failures. - -## Console helper conventions - -Always use the helpers from `apm_cli.utils.console` — never raw `print()` or bare `click.echo()`. - -**Emojis are banned.** Never use emoji characters anywhere in CLI output — not in messages, symbols, help text, or status indicators. Use ASCII text symbols exclusively via `STATUS_SYMBOLS`. - -```python -from apm_cli.utils.console import ( - _rich_success, _rich_error, _rich_warning, _rich_info, _rich_echo -) - -_rich_success("Installed 3 APM dependencies") # green, bold -_rich_info(" └─ 2 prompts integrated → .github/prompts/") # blue -_rich_warning("Config drift detected — re-run apm install") # yellow -_rich_error("Failed to download package") # red -_rich_echo(" [pkg-name]", color="dim") # dim, for verbose details -``` - -Use `STATUS_SYMBOLS` dict with `symbol=` parameter for consistent ASCII prefixes: -```python -_rich_info("Starting operation...", symbol="gear") # renders as "[*] Starting operation..." -``` - -## Output structure pattern - -Follow this visual hierarchy for multi-package operations: - -``` -[checkmark] package-name-1 # _rich_success — download/copy ok - [tree] 2 prompts integrated → .github/prompts/ # _rich_info — indented summary - [tree] 1 skill(s) integrated → .github/skills/ -[checkmark] package-name-2 - [tree] 1 instruction(s) integrated → .github/instructions/ - -── Diagnostics ── # Only if diagnostics.has_diagnostics - [warning] N files skipped — ... # Grouped by category - Run with --verbose to see details - -Installed 2 APM dependencies # _rich_success — final summary -``` - -## Content-awareness principle - -Before reporting changes, check if anything actually changed. Don't report no-ops. - -```python -# Bad — always copies and reports, even when content is identical -shutil.rmtree(target) -shutil.copytree(source, target) -_rich_info(f" └─ Skill updated") - -# Good — skip when content matches -if SkillIntegrator._dirs_equal(source, target): - continue # Nothing changed, nothing to report -``` - -## CommandLogger Architecture - -APM is a large and growing CLI with 10+ commands, 8+ integrators, and dozens of output sites. The logging architecture enforces **Separation of Concerns**: commands declare *what* happened; the logger decides *how* to render it. This keeps output consistent, testable, and evolvable without shotgun surgery across command files. - -### The three layers - -``` -┌─────────────────────────────────────────────────────┐ -│ Command layer (install.py, pack.py, audit.py …) │ -│ Calls: logger.success(), logger.tree_item(), … │ -│ NEVER calls: _rich_*, click.echo(), print() │ -├─────────────────────────────────────────────────────┤ -│ Logger layer (command_logger.py) │ -│ CommandLogger ← InstallLogger, future subclasses │ -│ Owns: verbose gating, symbol choice, indentation │ -│ Delegates to: _rich_* helpers │ -├─────────────────────────────────────────────────────┤ -│ Rendering layer (console.py) │ -│ _rich_echo, _rich_success, _rich_error, … │ -│ Owns: Rich/colorama fallback, color, STATUS_SYMBOLS │ -└─────────────────────────────────────────────────────┘ -``` - -Changes to output style (colors, symbols, indentation) happen in the **logger or rendering layer only** — command code is untouched. New output patterns (e.g. a tree sub-item, a package metadata line) become new logger methods, not ad-hoc format strings in commands. - -### Base class: `CommandLogger` - -`src/apm_cli/core/command_logger.py` — base for all commands. - -| Method | Purpose | When to use | -|--------|---------|-------------| -| `start(msg, symbol=)` | Operation start | Beginning of a command | -| `progress(msg, symbol=)` | Status update with `[i]` prefix | Mid-operation phase changes | -| `success(msg, symbol=)` | Green success | Operation completed | -| `warning(msg, symbol=)` | Yellow warning | User action needed | -| `error(msg, symbol=)` | Red error | Operation failed | -| `verbose_detail(msg)` | Dim text, verbose-only | Internal details (paths, hashes) | -| `tree_item(msg)` | Green text, no symbol prefix | `└─` sub-items under a package | -| `package_inline_warning(msg)` | Yellow text, verbose-only | Per-package diagnostic hints | -| `dry_run_notice(msg)` | `[dry-run]` prefix | Dry-run explanation | -| `auth_step(step, success, detail)` | Auth resolution step | Verbose auth tracing | -| `render_summary()` | Render DiagnosticCollector | End of command | - -### Subclass: `InstallLogger(CommandLogger)` - -Install-specific phases. Commands that don't need these use `CommandLogger` directly. - -| Method | Purpose | Output | -|--------|---------|--------| -| `validation_start(count)` | Start validation | `[*] Validating N package(s)...` | -| `validation_pass(name, present)` | Package OK | `[+] name` or `name (already in apm.yml)` | -| `validation_fail(name, reason)` | Package bad | `[x] name -- reason` | -| `resolution_start(count, lockfile)` | Start resolution | Context-aware install/update message | -| `download_complete(name, ref=, sha=, cached=)` | Package installed | `[+] name #tag @sha` or `(cached)` | -| `download_failed(name, error)` | Download error | `[x] name -- error` | -| `lockfile_entry(key, ref=, sha=)` | Lockfile verbose line | `key: locked at sha` / `pinned to ref` / omitted | -| `package_auth(source, token_type=)` | Auth source verbose | `Auth: source (type)` | -| `package_type_info(label)` | Package type verbose | `Package type: label` | -| `install_summary(apm, mcp, errors)` | Final summary | `Installed N APM dependencies.` | - -### When to add a new logger method - -If a command needs a new output pattern (new indentation level, new semantic meaning, new verbose gate), **add a method to CommandLogger or a subclass**. Signs you need a new method: - -- You're writing `_rich_echo(f" Something: {value}", color="dim")` in a command file -- You're checking `if logger.verbose:` before calling `_rich_echo` in a command -- You're formatting a string with specific indentation that other commands might reuse -- Multiple commands emit the same kind of line (e.g., file lists, auth info) - -### Rule: No direct `_rich_*` in commands - -Command functions must NOT call `_rich_info()`, `_rich_error()`, etc. directly. Use `logger.progress()`, `logger.error()`, etc. instead. The `_rich_*` helpers are **internal** to the logger and rendering layers. - -**Exception:** Rich tables and panels for display (not lifecycle logging) may use `console.print()` directly — these are data presentation, not status reporting. - -### Rule: Every command gets a `CommandLogger` - -Every Click command function must instantiate a `CommandLogger` (or subclass) and pass it to helpers: - -```python -@cli.command() -@click.option("--verbose", "-v", is_flag=True) -@click.option("--dry-run", is_flag=True) -def my_command(verbose, dry_run): - logger = CommandLogger("my-command", verbose=verbose, dry_run=dry_run) - logger.start("Starting operation...") - _do_work(logger=logger) - logger.render_summary() -``` - -### Rule: Verbose gating lives in the logger - -Never check `if verbose:` in command code. Use methods that gate internally: - -```python -# Bad — manual verbose check in command -if verbose: - _rich_echo(f" Auth: {source}", color="dim") - -# Good — logger handles the gate -logger.package_auth(source, token_type) # No-ops when not verbose -logger.verbose_detail(f" Path: {path}") # No-ops when not verbose -``` - -### DiagnosticCollector integration - -Access via `logger.diagnostics` (lazy-initialized). The collector owns the collect-then-render lifecycle: - -```python -# During operation — collect -diagnostics.skip(file, package=pkg_name) # Collision -diagnostics.overwrite(file, package=pkg_name) # Cross-package replacement -diagnostics.error(msg, package=pkg_name) # Failure -diagnostics.auth(msg, package=pkg_name) # Auth issue - -# Query during operation (e.g., for inline verbose hints) -count = diagnostics.count_for_package(pkg_name, category="collision") -if count > 0: - logger.package_inline_warning(f" [!] {count} files skipped") - -# After operation — render grouped summary -logger.render_summary() # Delegates to diagnostics.render_summary() -``` - -### Visual hierarchy contract - -Multi-package operations follow this tree structure: - -``` - [+] package-name #v1.0 @b0cbd3df # download_complete - Auth: git-credential-fill (oauth) # package_auth (verbose) - Package type: Skill (SKILL.md detected) # package_type_info (verbose) - └─ 3 skill(s) integrated -> .github/skills/ # tree_item - └─ 1 prompt integrated -> .github/prompts/ # tree_item - [!] 2 files skipped (local files exist) # package_inline_warning (verbose) - [+] another-package (cached) # download_complete - -── Diagnostics ── # render_summary - [!] 2 files skipped -- local files exist # Grouped by category - Use 'apm install --force' to overwrite - -[*] Installed 2 APM dependencies. # install_summary -``` - -Key rules: -- `[+]` package lines are the top-level anchors (green, no indent beyond 2-space) -- Verbose metadata (Auth, Package type) uses 4-space indent, dim color -- Tree items (`└─`) use 4-space indent, green color, no symbol prefix -- Inline warnings use 4-space indent, yellow color, verbose-only -- Diagnostics summary appears AFTER all packages, not inline (except verbose hints) - -### Scaling guidance - -As the CLI grows, this architecture scales by: -- **New commands**: Instantiate `CommandLogger`, use existing methods. Add subclass only if the command has distinct phases (like `InstallLogger`). -- **New output patterns**: Add methods to `CommandLogger`. Every command benefits. -- **New integrators**: Accept `diagnostics=` param, push to collector. No direct output. -- **Theme changes**: Modify rendering layer (`console.py`). Zero command changes. -- **Testing**: Mock `CommandLogger` in tests to assert semantic calls without parsing output strings. - -## Anti-patterns - -1. **Warning for non-actionable state** — If the user can't do anything about it, use `_rich_info` or defer to `--verbose`, not `_rich_warning`. - -2. **Inline warnings in loops** — Use `DiagnosticCollector` to collect, then render a grouped summary after the loop. - -3. **Missing `diagnostics` parameter** — When calling integrators, always pass `diagnostics=diagnostics` so warnings route to the deferred summary. - -4. **No emojis, ever** — Emojis are completely banned from all CLI output. Use ASCII text symbols from `STATUS_SYMBOLS` exclusively. This applies to messages, help text, status indicators, and table titles. - -5. **Inconsistent symbols** — Always use `STATUS_SYMBOLS` dict with `symbol=` param, not inline characters. - -6. **Walls of text** — Use Rich tables for structured data, panels for grouped content. Break up long output with visual hierarchy (indentation, `└─` tree connectors). - -7. **Direct `_rich_*` calls in commands** — Use `logger.start()`, `logger.progress()`, `logger.tree_item()` etc. The `_rich_*` helpers are internal to CommandLogger and console.py. Adding a `_rich_echo` call in a command file is a SoC violation. - -8. **Manual `if verbose:` checks** — Use `logger.verbose_detail()`, `logger.package_auth()`, or other verbose-gated methods. The logger owns the gate. - -9. **Manual `if dry_run:` checks** — Use `logger.should_execute` or `logger.dry_run_notice()`. - -10. **Format strings for indentation in commands** — Don't write `f" Auth: {source}"` in command code. Use `logger.package_auth(source)` which owns the indent level. When a new indentation pattern is needed, add a method to CommandLogger. - -11. **Re-creating shared objects per iteration** — Expensive objects like `AuthResolver` should be created once before loops and reused per-package. The logger and diagnostics collector are already singletons per command invocation. - -12. **Using `logger.progress()` for tree sub-items** — `progress()` adds a `[i]` symbol prefix. Tree continuation lines (`└─`) should use `logger.tree_item()` which renders with no symbol. diff --git a/.agents/skills/devx-ux/SKILL.md b/.agents/skills/devx-ux/SKILL.md deleted file mode 100644 index 172e7b8d..00000000 --- a/.agents/skills/devx-ux/SKILL.md +++ /dev/null @@ -1,30 +0,0 @@ ---- -name: devx-ux -description: >- - Activate when designing or modifying CLI command surfaces, command help - text, install/init/run flows, error wording, or first-run experience - in the APM CLI -- even when the user does not say "UX" explicitly. ---- - -# Developer Tooling UX Skill - -[Developer Tooling UX expert persona](../../agents/devx-ux-expert.agent.md) - -## When to activate - -- Changes to `src/apm_cli/cli.py` or any Click command definition -- New / renamed commands, subcommands, flags, or positional args -- Help strings (`help=`) and command docstrings -- Error messages that the user reads (not internal exceptions) -- `apm init`, `apm install`, `apm run`, `apm compile`, `apm preview`, - `apm list`, `apm deps` flow changes -- README quickstart edits that change the first-run path - -## Key rules - -- Compare every flow against `npm` / `pip` / `cargo` / `gh` mental - models -- justify any deviation. -- Default output is for humans; `--verbose` is for agents. -- Every error names the failure, the cause, and one next action. -- Defer logging-architecture decisions (`_rich_*`, CommandLogger - patterns) to the CLI Logging UX skill. diff --git a/.agents/skills/docs-impact-architect/SKILL.md b/.agents/skills/docs-impact-architect/SKILL.md deleted file mode 100644 index 2bc2c997..00000000 --- a/.agents/skills/docs-impact-architect/SKILL.md +++ /dev/null @@ -1,149 +0,0 @@ ---- -name: docs-impact-architect -description: >- - Use this skill when the docs-impact-classifier returns a structural - verdict, signalling that the documentation TOC must change to - accommodate the PR. Proposes TOC deltas (new pages, moves, - merges) and emits new-page outline stubs that the doc-sync panel - later fleshes out. Holds the 3-promise narrative (consume / - produce / govern) and the persona ramps as hard constraints. ---- - -# docs-impact-architect - -Single responsibility: when the classifier says a PR needs -structural docs changes (new page, page move, TOC reshape), design -the change and emit: - -1. A precise TOC delta (added pages, moved pages, retired pages) -2. New-page outline stubs (slug, title, persona, promise, H2 sections, key examples) -3. The persona-ramp impact (which ramp gains/loses a stop) - -You are NOT the writer (doc-writer owns prose). You are the **TOC -architect**. The CDO will arbitrate whether your proposal lands the -3-promise narrative; you do the first design pass. - -## When to invoke - -The docs-sync orchestrator invokes you ONLY when the classifier -returned `verdict: structural`. For `no_change` or `in_place` you -don't run. - -## Inputs - -- `structural_proposal` from the classifier (a sketch you refine) -- The PR diff (`gh pr diff $PR`) -- `.apm/docs-index.yml` (full corpus map) -- The PR description (for author-stated intent) - -## Step 1: read the corpus map, not the corpus - -Load `.apm/docs-index.yml` entirely. Inspect `chapters[]`, `pages[]`, -`promises[]`. This is your map. You do NOT read the 100+ page corpus -unless a specific page is implicated by the classifier's sketch. - -## Step 2: classify the structural shape - -Match the PR's surface change to one of these structural shapes: - -| Shape | Pattern | Example | -|---|---|---| -| **NEW CAPABILITY** | A new CLI verb, primitive type, or schema concept the docs have no slot for | `apm pack --format wheel` adds a new package format | -| **EXPANDED CAPABILITY** | An existing concept grows in scope and the current page can't hold it | `apm install` gains a registry-proxy mode that needs its own sub-page | -| **DEPRECATED CAPABILITY** | A removed CLI verb, flag, or concept; existing pages need to be retired or rewritten | A flag is removed; tutorial pages still teach it | -| **CONCEPT SPLIT** | One concept becomes two distinct concepts; one page becomes two | `apm audit` splits into `audit` and `audit ci` | -| **CONCEPT MERGE** | Two concepts unify; two pages should become one | `apm pack` and `apm bundle` merge into one verb | -| **RAMP REORG** | The PR's surface change shifts a concept across promises (e.g. an enterprise feature becomes consumer-default) | Policy enforcement moves from enterprise to consumer default behaviour | - -The structural shape drives the TOC delta shape. - -## Step 3: design the TOC delta - -For each new page proposed, fill in: - -```yaml -new_page: - slug: docs/src/content/docs//.md - title: "" - persona: consumer | producer | enterprise | cross - promise: 1 | 2 | 3 | cross - parent_chapter: - h2_sections: - - "## Why " # OPTIONAL -- skip unless concept is genuinely new - - "## How to " # REQUIRED -- code first - - "## Reference" # OPTIONAL -- flag/option table - - "## Troubleshooting" # OPTIONAL -- only if known footguns - bridges: - incoming: # which existing pages should link TO this - - {from: , link_text: } - outgoing: # which existing pages should this link FROM - - {to: , link_text: } - ramp_impact: >- - one-paragraph description of how this changes the - ramp: which step it slots into, whether it adds a stop or - replaces an existing one -``` - -For each moved/retired page: - -```yaml -moved_page: - from: - to: - redirect_rationale: - -retired_page: - slug: - reason: - redirect_to: # MUST exist; orphaning pages breaks SEO -``` - -## Step 4: validate against the 3-promise narrative - -Apply these hard rules. If any fails, redesign: - -1. **Every page belongs to exactly one promise.** Cross-cutting pages (integrations, troubleshooting, reference) are explicitly marked `promise: cross`. If a new page straddles two promises, split it OR park it under `cross`. -2. **Consumer pages don't pre-teach producer concepts.** A consumer page may LINK to producer; it may not embed producer prose. -3. **Producer pages don't pre-teach enterprise concepts.** Same rule, one promise down. -4. **No page is orphaned from the TOC.** Every new page has a `parent_chapter` and at least one `incoming` bridge. -5. **No retired page lacks a `redirect_to`.** Search engines will index the old URL for months; the redirect is the SEO contract. - -## Step 5: emit the architect report - -Return JSON: - -```json -{ - "structural_shape": "NEW CAPABILITY" | "EXPANDED CAPABILITY" | "DEPRECATED CAPABILITY" | "CONCEPT SPLIT" | "CONCEPT MERGE" | "RAMP REORG", - "toc_delta": { - "new_pages": [...], - "moved_pages": [...], - "retired_pages": [...], - "chapter_changes": [...] - }, - "promise_validation": { - "all_pages_single_promise": true | false, - "no_orphans": true | false, - "no_unredirected_retires": true | false, - "concerns": [] - }, - "downstream_in_place_pages": ["..."], - "rationale": "<2-3 sentence summary of why this structural delta and not alternatives>" -} -``` - -`downstream_in_place_pages[]` is the handoff to the localizer -- after -the architect approves the TOC, the localizer plans in-place edits -to existing pages that REFERENCE the new structure. - -## Output contract - -Return a SINGLE JSON document matching the schema in Step 5 as the -final message of your task. No prose around the JSON. - -## Anti-patterns - -- Inflating new-page counts to seem thorough. The minimal true delta wins. -- Skipping the promise-validation step. The CDO will catch it; better to self-catch. -- Designing a new chapter when an existing chapter has room. Always prefer extending over creating. -- Forgetting `redirect_to` on retired pages. SEO debt is the silent corpus killer. diff --git a/.agents/skills/docs-impact-classifier/SKILL.md b/.agents/skills/docs-impact-classifier/SKILL.md deleted file mode 100644 index 70a1f2f1..00000000 --- a/.agents/skills/docs-impact-classifier/SKILL.md +++ /dev/null @@ -1,154 +0,0 @@ ---- -name: docs-impact-classifier -description: >- - Use this skill to classify the documentation impact of a pull - request diff, returning one of three verdicts -- no-change, - in-place edit, or structural change -- with bounded LLM cost. - Activate as a sibling skill of docs-sync; the orchestrator calls - this first, before any panel spawn, to keep cost floor at 1 LLM - call when no docs work is needed. Reads .apm/docs-index.yml as - the corpus map; never reads the full corpus. ---- - -# docs-impact-classifier - -Single responsibility: given a PR diff and the `.apm/docs-index.yml` -corpus map, emit ONE classification verdict. - -This skill is the cost gate for the entire docs-sync system. ~70% of -PRs should exit at verdict `no_change` with zero panel spawn. - -## Architecture - -This is a 3-layer funnel inside a single skill invocation: - -- **L0 deterministic path gate** -- pure file-path matching, no LLM. -- **L1 symbol extraction + corpus grep** -- pure text processing, no LLM. -- **L2 LLM classifier** -- bounded ~8 KB context envelope, 1 call. - -The skill returns the verdict from the earliest layer that can decide. - -## Step 1: L0 deterministic path gate (no LLM) - -Read `.apm/docs-index.yml` to load `no_impact_paths[]` and -`user_surface_paths[]`. Get the changed file list from the PR diff -(`gh pr diff --name-only`). - -``` -if every changed file matches no_impact_paths AND none match user_surface_paths: - return {verdict: "no_change", confidence: "high", source: "L0", scope_pages: []} -``` - -This handles: -- Test-only PRs (`tests/**`) -- CI workflow PRs (`.github/workflows/**`) -- Doc-only PRs (`docs/**`) -- out of scope, docs-sync doesn't review docs PRs -- Primitive-only PRs (`.apm/**`) -- Script and meta PRs - -Expected hit rate: ~70% of PRs short-circuit here. - -## Step 2: L1 symbol extraction + corpus grep (no LLM) - -If L0 did not exit, extract user-observable symbols from the diff: - -- **CLI command names** -- grep diff for `^@click.command`, `^@cli.command`, or any `apm ` mention in added/removed lines. -- **Flag names** -- grep diff for `^@click.option`, `--[a-z-]+` patterns. -- **Public API symbols** -- added/removed `def ` in `src/apm_cli/__init__.py` or `src/apm_cli/api/**`. -- **Schema keys** -- added/removed keys in `apm.yml`, `apm.lock.yaml`, `apm-policy.yml` parsers. -- **Error strings** -- added/removed string literals in user-facing error paths (look for `_rich_error`, `click.echo`, `raise ... Error(`). - -For each extracted symbol, consult `.apm/docs-index.yml#symbol_index` -to find the documented pages. Collect all hits into `candidate_pages[]`. - -Also `grep -rn docs/src/content/docs/` for symbols NOT in -the index (catches drift between index and corpus). - -## Step 3: L2 LLM verdict (1 call, bounded context) - -If L1 found zero candidate pages AND zero schema/CLI/flag changes: -return `{verdict: "no_change", confidence: "medium", source: "L1", scope_pages: []}`. - -Otherwise, invoke the doc-analyser persona with EXACTLY this context -envelope (must fit in ~8 KB tokens): - -- PR title + body (first 500 chars) -- Diff stats (`gh pr diff --stat` output) -- `.apm/docs-index.yml` (the whole file; it's ~8 KB seeded, may grow) -- L1 candidate pages with +/-5 lines of context per hit -- Path-classification summary from L0 -- **`pr_doc_diff_paths[]`**: the list of paths under `docs/src/content/docs/**` - that the PR itself already modifies (drives the `in_place_resolved` - downgrade rule in "In-place-resolved detection" below). - -Ask doc-analyser to return JSON matching this schema: - -```json -{ - "verdict": "no_change" | "in_place_resolved" | "in_place" | "structural", - "confidence": "low" | "medium" | "high", - "scope_pages": ["docs/src/content/docs/..."], - "structural_proposal": { - "new_pages": [{"slug": "...", "rationale": "..."}], - "moved_pages": [{"from": "...", "to": "..."}], - "toc_changes": "" - }, - "reasoning": "" -} -``` - -`structural_proposal` is populated only when verdict is `structural`. -`scope_pages` is populated for `in_place` and `structural` verdicts. - -## Verdict semantics - -| Verdict | Meaning | Panel size | Cost | -|---|---|---|---| -| `no_change` | No user-observable surface changed | 0 panel spawns | ~0-1 LLM call | -| `in_place_resolved` | Doc impact existed, but the PR's OWN diff already patches every page in `scope_pages` -- author already did the work | 0 panel spawns; skill emits NO advisory | ~1 LLM call | -| `in_place` | One to a few pages need a paragraph or section update; no new pages, no TOC change | N candidate pages x (doc-writer + python-architect) + editorial-owner + growth-hacker + CDO | ~6-12 LLM calls | -| `structural` | A new page is needed, OR an existing page should be split/merged, OR the TOC needs to change to fit a new concept | architect first (TOC delta), then in-place panel for affected pages | ~10-15 LLM calls | - -## In-place-resolved detection (false-alarm killer) - -BEFORE returning `in_place`, intersect your `scope_pages[]` with the -list of files the PR itself touches under `docs/**` (provided to you -by the orchestrator under `pr_doc_diff_paths[]`). If EVERY scope page -already appears in `pr_doc_diff_paths`, downgrade to `in_place_resolved` -and emit `reasoning` of the form "Author already patched ". -This is the well-behaved-author path; the skill stays silent. - -If only SOME scope pages are pre-patched, keep `in_place` and list the -REMAINING (unpatched) pages in `scope_pages[]`. Note the pre-patched -ones in `reasoning` for transparency. - -## Rename / breaking-change heuristic (PR 1244 class) - -When the L1 layer reports an ADDED public symbol that matches an -EXISTING public symbol's name in the corpus (e.g. PR adds `apm update` -but `apm update` already appears in 9 docs pages with different -semantics), this is a RENAME or BREAKING SEMANTIC CHANGE. Bias toward -`structural` (not `in_place`): -- the existing page describing the OLD semantics may need to SPLIT - into two pages (old verb under new name + new verb keeping old name) -- the TOC may need a NEW reference page for the renamed verb -- every passing mention in the corpus needs verification - -Do NOT collapse a rename into `in_place` just because the affected -pages already exist. The shape of the work is structural even when no -new page is strictly required. - -## Anti-patterns (verdict shape errors) - -- Returning `in_place` with empty `scope_pages` -- invalid; orchestrator will reject. -- Returning `structural` without `structural_proposal` -- invalid. -- Returning `in_place` when EVERY scope page is in `pr_doc_diff_paths` -- should be `in_place_resolved`. -- Inflating `structural` to seem thorough -- the CDO will catch this. Return the minimal true verdict. -- Missing the rename heuristic above and emitting `in_place` for a verb-swap PR. -- Reading the corpus (the .md files themselves) at L2 -- context budget breach. You read the index, not the corpus. - -## Output contract - -Return a SINGLE JSON document matching the schema in Step 3 as the -final message of your task. No prose around the JSON. The -orchestrator parses your last message. diff --git a/.agents/skills/docs-impact-localizer/SKILL.md b/.agents/skills/docs-impact-localizer/SKILL.md deleted file mode 100644 index 61e2ba2d..00000000 --- a/.agents/skills/docs-impact-localizer/SKILL.md +++ /dev/null @@ -1,124 +0,0 @@ ---- -name: docs-impact-localizer -description: >- - Use this skill to translate a classifier's in-place verdict into a - precise, page-by-page work plan for the docs-sync panel. Activate - after docs-impact-classifier returns verdict in_place; reads the - candidate page list, fetches the actual page contents, narrows - scope to specific sections within each page, and emits the - per-page task brief the panel fans out against. ---- - -# docs-impact-localizer - -Single responsibility: given a list of candidate pages from the -classifier, produce a per-page task brief the docs-sync panel can -fan out against. - -You are NOT the verdict-maker (classifier owns that). You are NOT -the writer (doc-writer owns that). You are the **work planner**. - -## When to invoke - -The docs-sync orchestrator invokes you ONLY when the classifier -returned `verdict: in_place`. For `no_change` you don't run. -For `structural` the architect runs first; you may run after, scoped -to existing pages that need amendment. - -## Inputs - -- `scope_pages[]` from the classifier -- The PR diff (`gh pr diff $PR`) -- `.apm/docs-index.yml` (per-page metadata) -- Optional: the structural architect's TOC delta (if you run after - the architect on a structural verdict) - -## Step 1: load page contents - -For each path in `scope_pages[]`, read the file. Pages are typically -3-10 KB; total budget for this step is bounded by the candidate -count (the classifier should have kept it to <= 6). - -## Step 2: narrow scope inside each page - -For each page, identify the SPECIFIC section(s) that need to change: - -- Read the page's H2/H3 structure -- For each diff symbol from the classifier output, find the section - most directly documenting it -- Capture line ranges: `lines 120-145` not `the whole page` - -The output is a `sections_to_edit[]` per page, where each entry is: - -```yaml -page: docs/src/content/docs/consumer/install.md -sections_to_edit: - - section: "## From Git" - line_range: [120, 145] - diff_symbol: "--no-cache flag" - edit_kind: add | modify | remove - rationale: "the new --no-cache flag is documented nowhere; section already lists other flags so this is the natural home" -``` - -## Step 3: detect cross-page conflicts - -If two pages document the same symbol and the diff changes the -symbol's behaviour, BOTH pages need an edit AND they must stay -consistent. Flag this in the brief so the CDO synthesizer knows to -cross-check coherence between the two redrafts: - -```yaml -cross_page_constraint: - pages: [path1, path2] - shared_symbol: "apm install --target" - consistency_required: "both pages must reflect the same default value" -``` - -## Step 4: emit the per-page task brief - -Return JSON with this shape (one entry per page in `scope_pages[]`): - -```json -{ - "tasks": [ - { - "page": "docs/src/content/docs/consumer/install.md", - "persona_owner": "consumer", - "promise": 1, - "sections_to_edit": [ - { - "section": "## From Git", - "line_range": [120, 145], - "diff_symbol": "--no-cache flag", - "edit_kind": "add", - "rationale": "..." - } - ], - "verify_claims": [ - {"claim": "the flag is named --no-cache", "verify_with": "apm install --help"}, - {"claim": "the flag is documented in click.option decorator", "verify_with": "grep -n no-cache src/apm_cli/commands/install.py"} - ] - } - ], - "cross_page_constraints": [ - {"pages": [...], "shared_symbol": "...", "consistency_required": "..."} - ], - "estimated_panel_calls": 8 -} -``` - -The `verify_claims[]` per page is consumed by the python-architect -panelist -- it tells the verifier WHICH claims need a S7 tool-call -check (run `apm install --help`, grep the source) rather than -prose-trusting. - -## Output contract - -Return a SINGLE JSON document matching the schema in Step 4 as the -final message of your task. No prose around the JSON. - -## Anti-patterns - -- Selecting whole pages when one section suffices (inflates context per panelist). -- Skipping `verify_claims[]` -- that's the S7 tool-bridge hook; the verifier needs it. -- Inventing pages not in `scope_pages[]` -- that's the classifier's job, not yours. If you think the classifier missed a page, return an extra field `localizer_concern` instead of expanding scope unilaterally. diff --git a/.agents/skills/docs-sync/SKILL.md b/.agents/skills/docs-sync/SKILL.md deleted file mode 100644 index a5d6e875..00000000 --- a/.agents/skills/docs-sync/SKILL.md +++ /dev/null @@ -1,238 +0,0 @@ ---- -name: docs-sync -description: >- - Use this skill whenever a pull request is opened, reopened, or - synchronized in microsoft/apm to assess whether and how the - documentation corpus must change to stay truthful with the - proposed code change. Activate even when the PR title or body - says nothing about docs -- the skill must run on every PR to - detect silent drift between code and docs. Classifies impact - as no-change, in-place edit (one to a few paragraphs), or - structural change (new page or TOC reshape), then orchestrates - a CDO + doc-writer + python-architect + editorial-owner + - growth-hacker loop to produce a patch-ready advisory. Does NOT - review code quality, security, or test coverage. Does NOT - auto-merge or auto-push doc edits. ---- - -# docs-sync -- per-PR documentation impact panel - -The docs corpus drifts silently and constantly. This skill catches -drift at PR-open time, classifies its impact, and orchestrates a -persona panel to produce a patch-ready advisory comment. - -The pattern is **A1 PANEL + B1 FAN-OUT/SYNTHESIZER + A8 ALIGNMENT -LOOP**. The classifier is the cost gate (~70% of PRs short-circuit -to no-change with ~1 LLM call). When the panel does fan out, every -agent reads a bounded context (~10 KB) -- never the full corpus. - -This skill is ADVISORY. It does not gate merge, apply verdict -labels, or push to the contributor's fork. The orchestrator is the -sole writer to the PR: exactly one comment per run (idempotent -edit-in-place), plus optional label sweeps. - -## Architecture invariants - -- **Cost ceiling: 15 LLM calls per run.** Hard-wired. The orchestrator refuses to spawn beyond. Header prints `N/15` for observability. -- **Single-writer interlock.** Only the orchestrator writes. Panelist subagents return JSON; they MUST NOT call any `gh` write command, post comments, or touch PR state. -- **Idempotent comment.** Exactly one comment per run, with a stable header `## Docs sync advisory`. Re-runs edit-in-place using `gh pr comment --edit-last`. -- **No fork-write.** Companion docs PRs (only on structural verdict with `docs-sync-confirm` label) open from a bot branch in the BASE repo; never pushed to the contributor's fork. -- **Index-not-corpus reads.** Every classifier and architect agent reads `.apm/docs-index.yml`, NOT the corpus itself. The corpus is sampled only by the localizer (which reads the specific candidate pages) and by per-page panelists (which read one page each). -- **S7 deterministic tool bridge.** The python-architect panelist MUST run real `apm --help`, `grep`, and `python -c` commands to verify doc claims, never assert from prose. - -## Roster - -| Role | Agent | Always active? | -|---|---|---| -| Classifier | [doc-analyser](../../agents/doc-analyser.agent.md) inside [docs-impact-classifier](../docs-impact-classifier/SKILL.md) | Yes (every run) | -| Localizer | [docs-impact-localizer](../docs-impact-localizer/SKILL.md) | Only on `in_place` verdict | -| Architect | [docs-impact-architect](../docs-impact-architect/SKILL.md) | Only on `structural` verdict | -| Writer | [doc-writer](../../agents/doc-writer.agent.md) | Per candidate page (fan-out) | -| Verifier | [python-architect](../../agents/python-architect.agent.md) | Per candidate page (fan-out, S7) | -| Editorial | [editorial-owner](../../agents/editorial-owner.agent.md) | Once across all redrafts | -| Growth | [oss-growth-hacker](../../agents/oss-growth-hacker.agent.md) | Once across all redrafts | -| Synthesizer | [cdo](../../agents/cdo.agent.md) | Once, with ALIGNMENT LOOP up to 3 redrafts | - -## Topology - -``` - docs-sync SKILL (orchestrator thread) - | - Step 1: classify (1 LLM call, may exit here) - | - v - verdict? - / | \ - no-change in-place structural - | | | - EXIT | architect (TOC delta) - | | - +----<-----+ - | - Step 2: localize (1 LLM call) -- per-page task brief - | - Step 3: FAN-OUT panel via task tool - | - +----+----+----+----+ - v v v v v - writer verify edit growth - x N x N once once - (parallel; each <=10 KB context) - | - Step 4: schema-validate returns - | - Step 5: CDO synthesize (1 LLM call) - | - agree? - / | \ - revise (N<=3 redrafts) | agree - | - Step 6: emit ONE comment via safe-outputs.add-comment - Step 7: OPTIONAL companion docs PR (only if structural AND - `docs-sync-confirm` label present) -``` - -## Execution checklist - -### Step 1 -- Classify - -Spawn ONE task: load the `docs-impact-classifier` skill, pass it the -PR number. It returns the classifier JSON. - -Validate the JSON against `assets/classifier-return-schema.json`. -On schema failure, abort the run with a comment explaining the -internal error. - -If verdict is `no_change`: skip to Step 6 with a brief advisory -("No docs impact detected. Reason: . LLM calls: 1/15.") - -### Step 2 -- Localize (in_place) or Architect (structural) - -For `in_place`: spawn ONE task that loads the -`docs-impact-localizer` skill with the classifier output. Returns -per-page task briefs. - -For `structural`: spawn ONE task that loads the -`docs-impact-architect` skill with the classifier output. Returns -TOC delta + new-page outlines + downstream in-place pages. THEN -spawn the localizer for those downstream pages. - -### Step 3 -- Fan-out panel - -**Cascade-size mitigation (PR 1244 class).** If `scope_pages[]` has ->8 entries, the per-page fan-out at one writer call per page would -approach the 15-call ceiling with no headroom for verifier redrafts. -BEFORE spawning, group `scope_pages[]` into SECTIONS: - -- Pages under the same TOC section (e.g. all `consumer/**`) with the - SAME conceptual fix (e.g. "rename apm update -> apm self-update in - every mention") become ONE writer task with a `pages_in_section[]` - array in its brief. -- A 9-page rename cascade collapses to 2-3 section writer tasks. - -The python-architect verifier still runs per `verify_claims[]` (not -per page), because S7 evidence is keyed on claims, not pages. - -For each page-or-section in the per-page task brief, spawn TWO parallel tasks: - -1. **doc-writer** task -- drafts the patch for that page's (or section's) specific edits. Output: JSON with `before:`, `after:` for each location. -2. **python-architect** task -- for each `verify_claims[]` in the page brief, run the actual command (S7 tool bridge: `apm --help`, `grep -n src/`). Output: JSON with `claim: verified | refuted | inconclusive` per claim. - -In parallel with the per-page fan-out, spawn ONCE each: - -3. **editorial-owner** task -- receives ALL writer drafts, returns tone fixes. -4. **oss-growth-hacker** task -- receives ALL writer drafts, returns ramp-clarity notes (does this read well to a cold OSS visitor). - -All panelist tasks return JSON matching `assets/panelist-return-schema.json`. -Schema-validate every return; on failure, abort. - -### Step 4 -- Validate - -Cross-check: - -- Every `verify_claims` from a python-architect comes back `verified` or `inconclusive` (never `refuted`). If any are `refuted`, the doc-writer's draft is wrong; re-run the writer for that page with the refutation as context. -- Cross-page constraints from the localizer are honored across all writer drafts. -- All drafts are ASCII-only (per repo encoding rule). - -### Step 5 -- CDO synthesize - -Spawn ONE task: load the `cdo` persona with the full panel return -(writer drafts + verifier reports + editorial notes + growth notes -+ classifier verdict + (architect output if structural)) and -`.apm/docs-index.yml`. - -The CDO returns one of three verdicts: - -- `agree`: ship. Proceed to Step 6. -- `revise`: re-spawn the writer panelists with the CDO's specific - concerns as additional context. Re-run the editorial and growth - passes if needed. Bounded N <= 3 redrafts. Increment a redraft - counter; if it hits 3 and CDO still disagrees, ship with - `cdo_disagreement_noted: true`. -- `ship_with_disagreement`: ship as-is with the disagreement - surfaced in the comment for the maintainer to weigh. - -### Step 6 -- Emit ONE comment - -Render `assets/advisory-comment-template.md` with the final results. -Write it via `safe-outputs.add-comment`. Header is exactly -`## Docs sync advisory` (stable for idempotent edit-in-place). - -The comment MUST include the cost header: - -``` -Verdict: * Pages affected: N * LLM calls: M/15 * Took: Xs -``` - -### Step 7 -- Optional companion PR - -Only on `structural` verdict AND `docs-sync-confirm` label present -on the PR (the A9 SUPERVISED EXECUTION boundary; the maintainer -ratifies the structural proposal before any PR is opened). - -If both conditions hold: - -1. Branch name: `docs-sync/companion-` in the BASE repo. -2. Apply the doc-writer drafts as a commit on that branch. -3. Apply the architect's TOC delta (`.apm/docs-index.yml` entries + - new page files + redirects on retired pages). -4. Open a draft PR linked to the original PR, with the advisory - comment text as the PR body. -5. Reference the companion PR in the advisory comment. - -This step is intentionally GATED. The default behaviour (no -`docs-sync-confirm` label) is to recommend the patches in the -comment without opening a PR. - -## Cost accounting - -The orchestrator maintains a running LLM-call counter: - -| Step | Min calls | Max calls | -|---|---|---| -| Step 1 classify | 1 | 1 | -| Step 2 localize/architect | 0 | 2 | -| Step 3 fan-out (N pages) | 0 | 2N + 2 | -| Step 5 CDO | 0 | 1 + 3 redrafts | -| Total | 1 | 15 | - -If the counter would exceed 15, the orchestrator stops spawning, -ships the partial result with `cost_ceiling_hit: true`, and the -comment surfaces the truncation. - -## Anti-patterns - -- Reading the corpus instead of the index. Context budget breach. -- Letting panelists post comments. Single-writer interlock violation. -- Ignoring `refuted` verify_claims. That's silent drift you're shipping. -- Skipping the CDO synthesis on "obvious" in-place patches. The bridges still matter. -- Auto-opening companion PRs without the confirm label. Removes the human ratification. -- Re-running on every push (synchronize). Wasteful. Re-apply the trigger label for re-run. - -## Operating modes - -- **Rung 1 (label-gated, default)**: triggered by `docs-sync` label on PR. Maintainer opts in. -- **Rung 2 (default-on)**: triggered on every `pull_request_target` event. Enabled only after shadow validation. - -The workflow file controls which rung is active. The skill body is -identical for both. diff --git a/.agents/skills/docs-sync/assets/advisory-comment-template.md b/.agents/skills/docs-sync/assets/advisory-comment-template.md deleted file mode 100644 index 42b40eaa..00000000 --- a/.agents/skills/docs-sync/assets/advisory-comment-template.md +++ /dev/null @@ -1,106 +0,0 @@ -## Docs sync advisory - -Verdict: **{{ verdict }}** * Pages affected: {{ pages_affected_count }} * LLM calls: {{ llm_calls_used }}/15 * Took: {{ elapsed_seconds }}s - -{{ #if cost_ceiling_hit }} -> WARNING: Hit the 15 LLM call ceiling. Result is partial; see `cost_ceiling_hit: true` flag. -{{ /if }} - -{{ #if cdo_disagreement_noted }} -> NOTE: CDO disagreement after 3 redraft rounds. Maintainer judgement needed; see "Open concerns" below. -{{ /if }} - -### Summary - -{{ summary_paragraph }} - -{{ #if pages_affected_count == 0 }} - -No documentation changes needed for this PR. - -{{ classifier_reasoning }} - -{{ else }} - -### Proposed patches - -{{ #each page_patches }} - -#### `{{ this.page }}` ({{ this.persona }} ramp, promise {{ this.promise }}) - -{{ #each this.sections }} - -**Section: {{ this.section }}** (lines {{ this.line_range }}) - -```diff -- {{ this.before }} -+ {{ this.after }} -``` - -Rationale: {{ this.rationale }} - -{{ #if this.verifications }} -Verified by: {{ this.verifications }} -{{ /if }} - -{{ /each }} - -{{ /each }} - -{{ /if }} - -{{ #if structural_proposal }} - -### Structural proposal - -{{ structural_proposal.summary }} - -**New pages:** - -{{ #each structural_proposal.new_pages }} -- `{{ this.slug }}` -- {{ this.title }} ({{ this.persona }} ramp). {{ this.rationale }} -{{ /each }} - -**Moved / retired:** - -{{ #each structural_proposal.moved_pages }} -- `{{ this.from }}` -> `{{ this.to }}` ({{ this.redirect_rationale }}) -{{ /each }} - -{{ #if structural_proposal.confirm_label_present }} - -A companion docs PR has been opened: {{ companion_pr_link }}. - -{{ else }} - -To open a companion docs PR with these changes, apply the `docs-sync-confirm` label to this PR. - -{{ /if }} - -{{ /if }} - -{{ #if open_concerns }} - -### Open concerns (from CDO) - -{{ #each open_concerns }} -- {{ this }} -{{ /each }} - -{{ /if }} - ---- - -
-How this advisory was produced - -- Classifier verdict: `{{ verdict }}` (confidence: {{ confidence }}, source: {{ classifier_source }}) -- Panel composition: {{ panel_composition }} -- Tool-verified claims: {{ verification_count }} ({{ verification_pass_count }} verified, {{ verification_refute_count }} refuted, {{ verification_inconclusive_count }} inconclusive) -- CDO redraft rounds: {{ cdo_redraft_rounds }}/3 - -This is an advisory comment from the `docs-sync` skill ([source](.apm/skills/docs-sync/SKILL.md)). It does not gate merge. The maintainer ships. - -Re-run by removing and re-applying the `docs-sync` label. - -
diff --git a/.agents/skills/docs-sync/assets/classifier-return-schema.json b/.agents/skills/docs-sync/assets/classifier-return-schema.json deleted file mode 100644 index 009dfa3c..00000000 --- a/.agents/skills/docs-sync/assets/classifier-return-schema.json +++ /dev/null @@ -1,54 +0,0 @@ -{ - "$schema": "http://json-schema.org/draft-07/schema#", - "title": "docs-impact-classifier return", - "type": "object", - "required": ["verdict", "confidence", "reasoning"], - "properties": { - "verdict": { - "type": "string", - "enum": ["no_change", "in_place_resolved", "in_place", "structural"], - "description": "no_change: no doc impact (true negative). in_place_resolved: doc impact existed but the PR's own diff already patched every affected page (silent success; skill emits NO advisory). in_place: existing pages need edits. structural: TOC change or new page required." - }, - "confidence": { - "type": "string", - "enum": ["low", "medium", "high"] - }, - "source": { - "type": "string", - "enum": ["L0", "L1", "L2"], - "description": "Which funnel layer produced the verdict." - }, - "scope_pages": { - "type": "array", - "items": {"type": "string"}, - "description": "Candidate doc pages affected. Empty for no_change." - }, - "structural_proposal": { - "type": ["object", "null"], - "properties": { - "new_pages": { - "type": "array", - "items": { - "type": "object", - "properties": { - "slug": {"type": "string"}, - "rationale": {"type": "string"} - } - } - }, - "moved_pages": { - "type": "array", - "items": { - "type": "object", - "properties": { - "from": {"type": "string"}, - "to": {"type": "string"} - } - } - }, - "toc_changes": {"type": "string"} - } - }, - "reasoning": {"type": "string"} - } -} diff --git a/.agents/skills/docs-sync/assets/panelist-return-schema.json b/.agents/skills/docs-sync/assets/panelist-return-schema.json deleted file mode 100644 index a2e1d118..00000000 --- a/.agents/skills/docs-sync/assets/panelist-return-schema.json +++ /dev/null @@ -1,68 +0,0 @@ -{ - "$schema": "http://json-schema.org/draft-07/schema#", - "title": "docs-sync panelist return", - "description": "Common shape for all panelist returns (doc-writer, python-architect verifier, editorial-owner, oss-growth-hacker).", - "type": "object", - "required": ["persona", "page"], - "properties": { - "persona": { - "type": "string", - "enum": ["doc-writer", "python-architect", "editorial-owner", "oss-growth-hacker"] - }, - "page": { - "type": ["string", "null"], - "description": "Path of the page this return covers. Null for editorial-owner/growth-hacker which return cross-page." - }, - "drafts": { - "type": "array", - "description": "doc-writer only: per-section before/after pairs.", - "items": { - "type": "object", - "properties": { - "section": {"type": "string"}, - "line_range": {"type": "array", "items": {"type": "integer"}}, - "before": {"type": "string"}, - "after": {"type": "string"} - } - } - }, - "verifications": { - "type": "array", - "description": "python-architect only: claim verification results from S7 tool calls.", - "items": { - "type": "object", - "properties": { - "claim": {"type": "string"}, - "command_run": {"type": "string"}, - "result": {"type": "string", "enum": ["verified", "refuted", "inconclusive"]}, - "evidence": {"type": "string"} - } - } - }, - "tone_fixes": { - "type": "array", - "description": "editorial-owner only: prose edits with before/after.", - "items": { - "type": "object", - "properties": { - "page": {"type": "string"}, - "before": {"type": "string"}, - "after": {"type": "string"}, - "rationale": {"type": "string"} - } - } - }, - "ramp_notes": { - "type": "array", - "description": "oss-growth-hacker only: cold-reader observations.", - "items": { - "type": "object", - "properties": { - "page": {"type": "string"}, - "concern": {"type": "string"}, - "fix": {"type": "string"} - } - } - } - } -} diff --git a/.agents/skills/docs-sync/evals/README.md b/.agents/skills/docs-sync/evals/README.md deleted file mode 100644 index 175db4ef..00000000 --- a/.agents/skills/docs-sync/evals/README.md +++ /dev/null @@ -1,35 +0,0 @@ -# docs-sync evals - -This directory holds the eval suite for the `docs-sync` skill, per -the genesis canonical evals doctrine (MODULE ENTRYPOINT primitive). - -## Files - -- `trigger-evals.json` -- 20 dispatch evals (10 should-trigger, - 10 should-NOT-trigger), 60/40 train/val split. The validation - split is the ship gate: rate >= 0.5 on should-trigger AND - < 0.5 on should-not-trigger. - -- `content-evals.json` -- 3 content scenarios (E1 surgical CLI - fix, E2 new flag, E3 new package format) exercised - with_skill vs without_skill to prove value-delta. - -## Ship gates - -The skill is ready to graduate from rung 1 (label-gated) to rung 2 -(default-on) when ALL of these pass: - -1. Trigger-eval val split: rate >= 0.5 on should-trigger AND - < 0.5 on should-not-trigger. -2. Content evals E1, E2, E3 each produce a measurable value-delta - between `with_skill` and `without_skill` runs. -3. Shadow-run on >= 5 recent real PRs in microsoft/apm with - no false-alarm advisories on test-only / CI-only PRs. -4. Cost ceiling (15 LLM calls) not hit on any shadow-run case. - -## Notes - -- Eval execution is currently manual. Future: tie into a CI job - similar to `apm-review-panel/evals/render_eval.py`. -- The shadow-run phase is the most important. Synthetic evals - cannot fully predict classifier accuracy on real PR diffs. diff --git a/.agents/skills/docs-sync/evals/content-evals.json b/.agents/skills/docs-sync/evals/content-evals.json deleted file mode 100644 index 487dba7a..00000000 --- a/.agents/skills/docs-sync/evals/content-evals.json +++ /dev/null @@ -1,74 +0,0 @@ -{ - "description": "Content evals for docs-sync. Each scenario is exercised with_skill (docs-sync loaded) and without_skill (no skill, just a generic doc reviewer). If outputs are indistinguishable, the skill is not adding value -- redesign or delete (genesis evals doctrine).", - "scenarios": [ - { - "id": "E1-surgical-cli-fix", - "label": "Surgical CLI fix -- no doc impact", - "setup": { - "pr_title": "fix: improve error message when apm install hits 404", - "diff_summary": "src/apm_cli/commands/install.py: change one error string from 'package not found' to 'package not found at '", - "files_changed": ["src/apm_cli/commands/install.py"], - "loc_changed": 3 - }, - "expected_verdict": "no_change", - "expected_cost_ceiling": 2, - "expected_panel_spawns": 0, - "value_delta_hypothesis": "Without the skill, a maintainer might wonder if docs need updating; with it, the L0 gate emits a clean 'no impact' advisory in <1 LLM call." - }, - { - "id": "E2-new-flag-added", - "label": "New flag added -- in-place edit on reference page", - "setup": { - "pr_title": "feat: add --no-cache flag to apm install", - "diff_summary": "src/apm_cli/commands/install.py: add click.option('--no-cache', is_flag=True). Update install logic to bypass the local cache.", - "files_changed": ["src/apm_cli/commands/install.py", "src/apm_cli/install/resolver.py"], - "loc_changed": 47 - }, - "expected_verdict": "in_place", - "expected_scope_pages": ["docs/src/content/docs/consumer/install.md"], - "expected_cost_ceiling": 8, - "expected_panel_spawns": 5, - "expected_panel_outputs": [ - "doc-writer drafts a flag-table entry for --no-cache", - "python-architect verifies via `apm install --help` that the flag exists and the description matches the draft", - "editorial-owner trims any marketing voice", - "growth-hacker checks the flag is mentioned in the consumer ramp, not just the reference", - "CDO confirms the patch fits the consumer promise" - ], - "value_delta_hypothesis": "Without the skill, the flag would ship undocumented and surface in the next user issue ('how do I bypass the cache?'). With the skill, the patch is attached to the PR comment ready to apply." - }, - { - "id": "E3-new-package-format", - "label": "New package format -- structural change", - "setup": { - "pr_title": "feat: add wheel format to apm pack", - "diff_summary": "src/apm_cli/commands/pack.py: add --format wheel support. New module src/apm_cli/pack/wheel.py. Update apm.yml schema to allow format: wheel.", - "files_changed": [ - "src/apm_cli/commands/pack.py", - "src/apm_cli/pack/wheel.py", - "src/apm_cli/models/apm_package.py" - ], - "loc_changed": 312 - }, - "expected_verdict": "structural", - "expected_structural_proposal": { - "new_pages": ["docs/src/content/docs/reference/package-formats/wheel.md"], - "in_place_pages": [ - "docs/src/content/docs/producer/pack-a-bundle.md", - "docs/src/content/docs/reference/package-types/index.md" - ] - }, - "expected_cost_ceiling": 14, - "expected_panel_spawns": 8, - "expected_panel_outputs": [ - "architect proposes new reference page slug, outlines H2 sections, identifies bridges", - "doc-writer drafts the new page outline and the in-place edits on the producer ramp", - "python-architect verifies via `apm pack --help` that --format wheel exists and runs apm pack --format wheel --dry-run on a fixture", - "editorial-owner ensures the new page reads in APM voice", - "growth-hacker checks the new format is referenced from the producer ramp index", - "CDO arbitrates whether 'wheel' belongs in reference/package-formats/ or producer/pack-a-bundle.md sub-section -- chooses based on 3-promise narrative" - ], - "value_delta_hypothesis": "Without the skill, the new format would either be undocumented (silent drift) or get one paragraph crammed into producer/pack-a-bundle.md (concept bloat). With the skill, the structural proposal is on the table at PR-open time and the maintainer ratifies via the docs-sync-confirm label." - } - ] -} diff --git a/.agents/skills/docs-sync/evals/trigger-evals.json b/.agents/skills/docs-sync/evals/trigger-evals.json deleted file mode 100644 index f0d29286..00000000 --- a/.agents/skills/docs-sync/evals/trigger-evals.json +++ /dev/null @@ -1,41 +0,0 @@ -{ - "description": "Trigger evals for the docs-sync skill dispatch description. 10 should-trigger + 10 should-NOT-trigger. 60/40 train/val split. Validation split is the ship gate (>=0.5 on should-trigger AND <0.5 on should-not-trigger).", - "should_trigger": { - "train": [ - "PR opened: adds --no-cache flag to apm install", - "PR opened: renames apm pack to apm bundle", - "PR opened: adds new package format wheel to apm pack", - "PR opened: removes the deprecated --legacy-resolver flag from apm install", - "PR opened: adds new schema field 'registry-proxy-url' to apm.yml", - "this PR changes the default value of apm.lock.yaml integrity field" - ], - "val": [ - "PR opened: refactors AuthResolver to support new GHE auth method, changes error messages", - "PR opened: adds new apm verb 'apm graph' that visualizes dependency tree", - "PR opened: changes the format of apm-policy.yml's allowed-sources field", - "PR opened: adds new primitive type 'workflow' to the producer authoring surface" - ] - }, - "should_not_trigger": { - "train": [ - "PR opened: refactor internal hashing helper, no public API change", - "PR opened: add unit tests for AuthResolver edge cases", - "PR opened: bump ruff dev dependency to latest", - "PR opened: fix typo in CHANGELOG.md", - "PR opened: update copilot-setup-steps.yml runner image", - "PR opened: rewrite README intro paragraph (docs-only PR)" - ], - "val": [ - "PR opened: extract _build_git_env helper into separate module (internal refactor)", - "PR opened: fix flaky integration test test_install_concurrent", - "PR opened: rewrite docs/src/content/docs/getting-started/quickstart.md (docs-only PR)", - "PR opened: update .github/instructions/changelog.instructions.md" - ] - }, - "notes": [ - "Docs-only PRs explicitly do NOT trigger -- docs-sync reviews CODE PRs for docs impact; docs PRs go through doc-writer review separately.", - "Pure refactors with no user-observable surface change must NOT trigger -- the L0 path gate should catch them.", - "Test-only PRs and CI-only PRs must NOT trigger.", - "The classifier may still emit no_change verdict on a borderline case; the dispatch eval here is about whether the SKILL is invoked, not the verdict." - ] -} diff --git a/.agents/skills/oss-growth/SKILL.md b/.agents/skills/oss-growth/SKILL.md deleted file mode 100644 index 5f56f274..00000000 --- a/.agents/skills/oss-growth/SKILL.md +++ /dev/null @@ -1,37 +0,0 @@ ---- -name: oss-growth -description: >- - Activate for OSS adoption work -- README conversion surfaces, - quickstart, templates, release announcements, contributor funnel, - story angles -- and any update to the maintained growth strategy at - WIP/growth-strategy.md. ---- - -# OSS Growth Skill - -[OSS growth hacker persona](../../agents/oss-growth-hacker.agent.md) - -## When to activate - -- README hero / quickstart / examples sections -- `docs/` content that affects first-run conversion -- `templates/` (starter projects shape the second-use experience) -- Release notes / launch posts / social copy -- Edits to `WIP/growth-strategy.md` -- Issue templates that affect the contributor funnel -- Any reviewed change that the CEO flags as having growth implications - -## Key rules - -- `WIP/growth-strategy.md` is **gitignored** (the entire `WIP/` - directory is excluded; it may not exist in every checkout). Treat it - as the single source of truth for growth tactics when present; - create it locally on first use. Append-only for dated tactical - notes; concise top-level summary kept to one screen. Never stage or - commit anything under `WIP/`. -- Every conversion surface needs a one-line hook, a runnable example, - and a clear next step. -- Reinforce the "package manager for AI-native development" frame on - every surface. Cut anything that dilutes it. -- Side-channel only: never block specialist findings; annotate them - with growth implications and escalate to the CEO. diff --git a/.agents/skills/pr-description-skill/SKILL.md b/.agents/skills/pr-description-skill/SKILL.md deleted file mode 100644 index e0c521f4..00000000 --- a/.agents/skills/pr-description-skill/SKILL.md +++ /dev/null @@ -1,340 +0,0 @@ ---- -name: pr-description-skill -description: >- - Use this skill to write the PR description (PR body) for any pull - request opened against microsoft/apm. Produces one self-sufficient - GitHub-Flavored Markdown artifact: TL;DR, Problem (WHY), Approach - (WHAT), Implementation (HOW), 1-3 validated mermaid diagrams, - explicit trade-offs, validation evidence, and a How-to-test - section -- with every WHY-claim backed by a verbatim quote from - PROSE or Agent Skills. Activate when the user asks to "write a PR - description", "draft a PR body", "open a PR", "fill in the PR - template", or any equivalent. ---- - -# PR Description Skill -- Anchored, Concise, Validated PR Bodies - -## When to use - -Trigger this skill on any of the following intents: - -- "write a PR description" -- "draft a PR body" -- "open a PR" / "open this PR" / "let's open the PR" -- "fill in the PR template" -- "summarize this branch as a PR" -- "create the PR write-up" - -Reusable for any PR against `microsoft/apm`. The output is one -markdown file that the orchestrator pastes into -`gh pr create --body-file` or surfaces to the maintainer. - -## Output charset rule (read this first) - -The repo-wide encoding rule at -`.github/instructions/encoding.instructions.md` constrains -**source files and CLI output** to printable ASCII because Windows -cp1252 terminals raise `UnicodeEncodeError` on anything else. PR -comments are NOT source code and NOT CLI output -- they are rendered -by GitHub's Primer engine, which expects UTF-8 GitHub-Flavored -Markdown. - -Two distinct rules therefore apply: - -1. **Source files in this bundle** (`SKILL.md`, `assets/*`) MUST - stay ASCII. They live in the repo and are subject to - `.github/instructions/encoding.instructions.md`. -2. **The PR body output the skill produces** MUST be UTF-8 - GitHub-Flavored Markdown. Use em dashes, smart punctuation, - alerts, collapsibles, task lists, and Unicode where it improves - readability. Mermaid diagram labels MAY use Unicode -- there is - no constraint here. The output is consumed by GitHub's renderer, - not by a Windows terminal. - -A previous version of this skill incorrectly required ASCII in the -PR body. That made the output unreadable: no alerts, no collapsibles -for long evidence, no em dashes, no smart quotes. Reviewers had to -scroll through hundreds of flat lines instead of scanning a body -shaped by GFM features. - -## Concision targets (hard ceilings) - -The skill aims for **150-220 lines** for a typical PR body. **300+ -lines is a smell, not a virtue**. If your draft exceeds 250 lines, -run a tightening pass: every sentence that does not change the -reviewer's understanding must be cut. - -Per-section ceilings (enforced by `assets/section-rubric.md`): - -| Section | Ceiling | -|---|---| -| TL;DR | 2-4 sentences | -| Problem (WHY) | max 6 bullets, max 3 quoted anchors total | -| Approach (WHAT) | a table OR 3-7 bullets; may be skipped if PR is purely additive (say "additive: see Implementation") | -| Implementation (HOW) | one short paragraph per file, OR a table; no prose walls | -| Diagrams | 1-3 mermaid blocks; every diagram preceded by a one-sentence legend | -| Trade-offs | 3-5 bullets; mechanical PRs may be 1-2 | -| Benefits | 3-5 numbered items, each measurable | -| Validation | copy-paste real command output; do not narrate | -| How to test | max 5 numbered steps | - -Long verbatim quote blocks, full file listings, and full validation -transcripts SHOULD live inside `
` so the body stays -scannable. - -## Core principles (with quoted anchors) - -Each rule the skill enforces is backed by a verbatim quote from one -of the two reference docs. If a rule below cannot be backed by a -quote, it is downgraded to a "should" with the reason given. - -1. **Self-sufficient body.** A reviewer must be able to read the PR - body and form an opinion without opening any other doc, issue, - or chat. Every WHY-claim cites the source doc inline; every - named file is qualified with what changed in it; every diagram - has a one-sentence legend. - - Anchor: Agent Skills, - ["agents pattern-match well against concrete structures"](https://agentskills.io/skill-creation/best-practices). - -2. **Anchored: every WHY-claim cites its source.** Every claim of - the form "this violates X" or "this satisfies Y" is followed by - a verbatim quoted phrase wrapped in a hyperlink to the source - page. Reproduce quotes character-for-character; do not paraphrase - inside link text. - - Anchor: PROSE, - ["Grounding outputs in deterministic tool execution transforms probabilistic generation into verifiable action."](https://danielmeppiel.github.io/awesome-ai-native/docs/prose/). - -3. **Cite-or-omit.** If a WHY-claim cannot be backed by a verbatim - quote, drop it or soften to a tradeoff statement. Never invent - justification. - - Anchor: Agent Skills, - ["Add what the agent lacks, omit what it knows"](https://agentskills.io/skill-creation/best-practices). - -4. **Visual aid where structure is non-trivial.** Any change that - touches more than one file or alters control flow SHOULD include - at least one mermaid diagram. Add a second only when the - relationships are non-trivial. Never add a third unless it earns - its place. Each diagram MUST be preceded by a one-sentence legend. - - Anchor: Agent Skills, - ["agents pattern-match well against concrete structures"](https://agentskills.io/skill-creation/best-practices). - -5. **Trade-offs explicit.** Address every non-obvious decision - (option chosen vs option rejected). For mechanical PRs this - section may be 1-2 bullets. For cross-cutting changes, surface - the rejected alternatives. - - Anchor: PROSE, - ["Favor small, chainable primitives over monolithic frameworks."](https://danielmeppiel.github.io/awesome-ai-native/docs/prose/). - -6. **Single artifact, no fluff.** One markdown file. No marketing - tone, no self-congratulation. TL;DR is at most four sentences. - - Anchor: Agent Skills, - ["When you find yourself covering every edge case, consider whether most are better handled by the agent's own judgment."](https://agentskills.io/skill-creation/best-practices). - -## GitHub-Flavored Markdown features the skill MUST use - -The PR body is rendered by GitHub's Primer engine. Use the features -that engine provides; do not flatten the output to plain text. - -- **Alerts** for high-signal callouts: - `> [!NOTE]`, `> [!TIP]`, `> [!IMPORTANT]`, `> [!WARNING]`, - `> [!CAUTION]`. Reference: - https://github.com/orgs/community/discussions/16925. -- **Collapsible sections** for long diffs, full validation output, - or appendix material: - - ``` -
Full audit output - - ...content... -
- ``` - - Use `
` only when the content answers the most - likely first reviewer question. -- **Task lists** for "How to test" sections: - `- [ ] Apply label, observe X`. -- **Tables with alignment**: `| col | :---: | ---: |` for matrices. -- **Permalink references** to specific lines in the diff: - `https://github.com/microsoft/apm/blob//path#L12-L34`. - -Long verbatim quote blocks, full file listings, and full validation -transcripts SHOULD live inside `
` so the body stays -scannable. - -## Required body structure - -| # | Section | Purpose | -|---|---------|---------| -| 1 | Title line | Imperative summary; first line `(): `, max 100 chars | -| 2 | TL;DR | 2-4 sentence executive summary | -| 3 | Problem (WHY) | Observed failure modes; max 6 bullets, max 3 quoted anchors | -| 4 | Approach (WHAT) | Table or 3-7 bullets; may say "additive: see Implementation" | -| 5 | Implementation (HOW) | One short paragraph per file or a table | -| 6 | Diagrams | 1-3 validated mermaid blocks, each with a legend; diagram type chosen per intent (`assets/mermaid-conventions.md`) | -| 7 | Trade-offs | 3-5 bullets (1-2 if mechanical) | -| 8 | Benefits | 3-5 numbered, measurable items | -| 9 | Validation | Real command output, ideally inside `
` if long; **MUST include the Scenario Evidence subsection** (`assets/scenario-evidence-rubric.md`) for any behavior-change PR -- maps each user-promise scenario this PR touches to the test that proves it works, tagged with the APM principle the scenario serves | -| 10 | How to test | Max 5 numbered or task-list steps | - -The Trade-offs (7) and How to test (10) sections are non-skippable -for any PR that changes more than docs. - -## Activation contract -- inputs the orchestrator MUST gather first - -Before invoking this skill, the orchestrator MUST have collected -all of the following. The skill MUST NOT invent facts not present -in these inputs. - -| Input | Source | Required | -|-------|--------|----------| -| Branch name (head) | `git rev-parse --abbrev-ref HEAD` | yes | -| Base ref | usually `main`; ask if unclear | yes | -| List of files changed | `git diff --name-status ...HEAD` | yes | -| Actual diff | `git diff ...HEAD` | yes | -| Commit messages on the branch | `git log --no-merges ..HEAD --oneline` | yes | -| CHANGELOG entry, if any | inspect `CHANGELOG.md` Unreleased section | yes | -| Linked issue / motivation | user-provided or referenced in commits | yes | -| Validation evidence | output of `apm audit --ci`, `uv run pytest`, or equivalent | yes | -| Scenario-test mapping | author-supplied or derived from diff: per user-promise scenario the PR touches, the test path proving it, plus the APM principle the scenario serves (taxonomy in `assets/scenario-evidence-rubric.md`) | conditional (required for any behavior-change PR; may be skipped for docs-only / asset-bump / pure-refactor per the rubric's skip clause, with the skip case stated in trade-offs) | -| Mirror parity check, if applicable | `apm install --target copilot` output | conditional | - -If any required input is missing, the orchestrator MUST stop and -collect it. This is a Progressive Disclosure boundary: -["Context arrives just-in-time, not just-in-case."](https://danielmeppiel.github.io/awesome-ai-native/docs/prose/). -Do not load `assets/pr-body-template.md` until the table above is -complete. - -## Execution checklist - -Run these steps in order. Tick each before moving on. - -1. [ ] Confirm every row of the activation contract is filled in. - Defense-in-depth gate: before drafting the body, confirm the - repo's lint contract is green (canonical commands and lifecycle - binding live in `.apm/instructions/linting.instructions.md`). If lint is red, - STOP, fix, re-run; a PR body claiming green CI while lint fails - is a credibility tax we refuse to take on. -2. [ ] Read the diff in full. Identify per-file change summary, - new files, deleted files, behavior changes at module - boundaries. -3. [ ] Load `assets/pr-body-template.md`. This is the only point - at which the template enters context. Progressive Disclosure - in action: - ["store them in `assets/` and reference them from `SKILL.md` so they only load when needed."](https://agentskills.io/skill-creation/best-practices). -4. [ ] Fill in the template top-to-bottom using only facts from - the activation contract. Every WHY-claim gets a verbatim - quoted anchor. If you cannot anchor a claim, drop it. -5. [ ] Generate 1-3 mermaid diagrams. **Before drafting any block, - load `assets/mermaid-conventions.md`** to pick the right - diagram type per intent (sequenceDiagram for execution flow, - flowchart LR for pipeline / architecture, stateDiagram-v2 for - state machines) and apply the boxing convention for NEW - behavior. Add a one-sentence legend above each diagram. -6. [ ] **Validate every mermaid block deterministically (see - below). Do NOT save the draft until every block validates.** -7. [ ] Load `assets/section-rubric.md` and run the self-check pass. - Validation loop pattern from Agent Skills: - ["do the work, run a validator (a script, a reference checklist, or a self-check), fix any issues, and repeat until validation passes."](https://agentskills.io/skill-creation/best-practices). -8. [ ] Run the line-count check. If the body exceeds 250 lines, - tighten until it fits 150-220. -9. [ ] Write the final body to a single file path provided by the - orchestrator (default: `.git/PR_BODY.md` or - session-state-relative). Return the path; do not paste the - body inline unless explicitly asked. - -## Mandatory mermaid validation step - -Run every mermaid block in the draft through `mmdc` and refuse to -save until all pass. - -```bash -# Extract mermaid blocks and validate each one. -# Requires: npx --yes -p @mermaid-js/mermaid-cli mmdc (one-shot, no global install needed) -awk '/^```mermaid/{n++; f=outdir"/diag"n".mmd"; getline; while($0 != "```") {print > f; getline}}' outdir=/tmp/mermaid-check pr-body-draft.md -for f in /tmp/mermaid-check/diag*.mmd; do - npx --yes -p @mermaid-js/mermaid-cli mmdc -i "$f" -o "${f%.mmd}.svg" --quiet || { echo "INVALID: $f"; exit 1; } -done -``` - -If `mmdc` reports any error, fix the diagram and re-run. The skill -MUST NOT save the draft until every mermaid block validates. - -### Diagram type and pitfalls reference - -The full diagram-type-by-intent table, canonical templates, and the -GitHub-renderer gotcha list (`mmdc` does NOT always catch GitHub -rejections) live in `assets/mermaid-conventions.md`. Load it whenever -a PR body needs a mermaid block. - -Critical drift-known gotcha (the one most likely to bite, captured -inline because it is not obvious from `mmdc` output): - -- **Square brackets in flowchart edge labels MUST be quoted.** - `A -->|[EXEC] work| B` parses on `mmdc` but is rejected by - GitHub's renderer (`Expecting 'TAGEND', ..., got 'SQS'`). Quote - the label: `A -->|"[EXEC] work"| B`. The same rule applies to - parentheses, colons, slashes, and pipes in edge labels. - -For everything else (semicolons in classDiagram links, `note right -of` closing rules, round brackets in node labels, inline -`:::cssClass` failing in classDiagram on GitHub), see -`assets/mermaid-conventions.md`. - -## Output contract - -- Exactly ONE markdown file is produced. -- The file is **UTF-8 GitHub-Flavored Markdown**. Em dashes, smart - quotes, Unicode in mermaid labels, alerts, and collapsibles are - all permitted and encouraged where they improve readability. -- Every mermaid block has been validated by `mmdc` and renders - without error. -- The cite-or-omit rule applies absolutely. -- The TL;DR is at most four sentences. -- The body ends with the trailer: - `Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>` - -## Anti-patterns flagged -- refuse these - -- **Posting unvalidated mermaid.** A parser error renders as raw - code on GitHub and signals carelessness. Validate every block - before saving. -- Pasting commit messages as the body. Commit messages are inputs, - not output. -- Marketing tone or self-congratulation ("this is a great - improvement", "significantly enhances", "best-in-class"). Strip - on sight. -- Diagrams without a legend, OR diagrams that fail `mmdc`. -- A TL;DR longer than four sentences. -- Skipping any required section because "the PR is small". A small - PR can have a one-line Implementation per file, but the section - header must still be present. -- Restating the diff line-by-line in Implementation. That is what - the Files Changed tab is for. -- Quoting a doc out of context. The self-check pass must verify - that the quoted phrase actually supports the claim. -- **Forcing ASCII-only on the PR body.** That rule applies to - source files and CLI output, not to Primer-rendered markdown. - See "Output charset rule" above. - -## Gotchas - -- **Do not restate the diff.** Implementation is for intent, risk, - and decisions -- not a textual re-rendering of the patch. -- **Do not quote out of context.** Re-read the surrounding paragraph - of the source doc before pasting a quote. -- **Verify the source URL still serves the quoted text.** If the - doc has been edited and the phrase no longer appears verbatim, - drop the citation or find a new anchor. -- **A doc-only PR still needs TL;DR, Problem, Validation, and - How-to-test.** "The PR is trivial" is not an exemption. -- **Long evidence belongs in `
`.** Reviewers should be - able to read the whole body in a single screen-and-a-half scroll - and expand evidence on demand. - -Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> diff --git a/.agents/skills/pr-description-skill/assets/mermaid-conventions.md b/.agents/skills/pr-description-skill/assets/mermaid-conventions.md deleted file mode 100644 index ddaa2926..00000000 --- a/.agents/skills/pr-description-skill/assets/mermaid-conventions.md +++ /dev/null @@ -1,229 +0,0 @@ -# Mermaid conventions for PR bodies - -Load this asset before drafting any mermaid block in a PR body. It -defines (a) which diagram TYPE to pick per intent, (b) the boxing / -styling vocabulary that highlights NEW behavior, and (c) the -GitHub-renderer gotchas that `mmdc` does NOT always catch. - -This asset is scoped to **PR bodies**. Architectural design diagrams -(component, thread fan-out, dependency graph) are owned by the -`genesis` skill and have a different convention set; do not conflate. - -## Diagram type by intent - -A PR body diagram answers ONE question. Pick the type that matches -the question; do not mix. - -| Reviewer's question | Diagram type | Boxing convention for "what changed" | -|---|---|---| -| Which jobs / participants run, in what order? (execution flow) | `sequenceDiagram` | `rect rgb(255, 247, 200)` block around new participant interactions; `Note over X` for invariants | -| What is the data / control pipeline? (stages, transformations) | `flowchart LR` | `subgraph` per stage; `classDef new stroke-dasharray: 5 5` for new stages; class assignment via `class N1,N2 new` | -| What does the state machine look like? | `stateDiagram-v2` | `note right of S: NEW` markers on changed states | -| How do components / files relate? (architecture) | `flowchart LR` | `classDef new stroke-dasharray: 5 5`; subgraphs for layers | -| Is there a true type hierarchy? (rare) | `classDiagram` | standalone `class Name:::cssClass` lines only -- inline `:::` on relationship lines fails on GitHub | - -### Default for "execution flow" PRs - -If the PR adds, removes, or reorders **jobs, steps, or third-party -action invocations** in a workflow, use `sequenceDiagram`. Reviewers -read it top-to-bottom as a temporal sequence; the participant lanes -make the boundary between "workflow", "job", and "external action" -explicit. A flat `flowchart TD` of the same content forces the reader -to reconstruct the temporal axis from arrows and is harder to scan. - -Use `rect rgb(...)` blocks to group the messages that the PR ADDS; -this gives the reviewer a single visual region to focus on without -hunting for `classDef`-marked nodes. - -### Default for "pipeline" PRs - -If the PR changes a data flow with discrete stages (parse -> validate --> render), use `flowchart LR` with one `subgraph` per stage. Mark -NEW stages with `classDef new stroke-dasharray: 5 5;` and assign -nodes via `class N1,N2 new;`. Avoid `flowchart TD` for left-to-right -pipelines; it wastes vertical space and breaks scanning rhythm. - -## Canonical templates - -### sequenceDiagram (execution flow) - -```mermaid -sequenceDiagram - participant W as Workflow - participant P as apm-prep job - participant A as apm job (matrix) - participant Ext as create-github-app-token - participant R as agent job - - W->>P: trigger - rect rgb(255, 247, 200) - Note over P,A: NEW: matrix fan-out per credential group - P->>A: groups[] (JSON) - A->>Ext: mint installation token (per group) - Ext-->>A: token - A->>A: pack and upload apm- - end - A-->>R: artifacts apm-* - Note over R: validate count vs manifest, restore via bundles-file -``` - -Conventions: - -- Each `participant` is a distinct actor (workflow, job, action). Do - NOT inline step-level work as participants -- those go inside the - sender's lane as `X->>X: action`. -- `->>` is a synchronous send; `-->>` is a return. Pick consistently. -- Wrap NEW interactions in `rect rgb(255, 247, 200)` (a soft yellow). - ASCII labels inside the rect are fine. -- `Note over` is for invariants ("single-writer", "must be true after - this point"), not for narrative. - -### flowchart LR (pipeline / architecture) - -```mermaid -flowchart LR - subgraph Parse[Parse] - P1[lockfile] - P2[manifest] - end - subgraph Validate[Validate] - V1[schema check] - V2[policy check]:::new - end - subgraph Render[Render] - R1[output] - end - P1 --> V1 - P2 --> V1 - V1 --> V2 - V2 --> R1 - classDef new stroke-dasharray: 5 5; - class V2 new; -``` - -Conventions: - -- One `subgraph` per logical stage; the subgraph label is the stage - name (capitalize for scanability). -- Mark NEW nodes with `classDef new stroke-dasharray: 5 5;` and a - separate `class N new;` assignment line (NOT inline `N:::new`, - which works in flowchart but is inconsistent with classDiagram and - hurts copy-paste portability). -- Edges carry verbs only when non-obvious. Default to unlabeled. -- Prefer `LR` for pipelines (left-to-right reads naturally). Use - `TD` only for tree-shaped hierarchies. - -### stateDiagram-v2 (state machine) - -```mermaid -stateDiagram-v2 - [*] --> Pending - Pending --> Resolving: install start - Resolving --> Cached: hit - Resolving --> Downloading: miss - note right of Downloading - NEW: per-group token mint - end note - Downloading --> Cached - Cached --> [*] -``` - -Convention: `note right of X` requires the multi-line form with -`end note` on its own line. Single-line `note right of X: text` is -NOT supported in `stateDiagram-v2` -- it parses elsewhere but -fails here. - -## GitHub-renderer gotchas (drift-known, mmdc does NOT always catch) - -These are renderer-level rejections that `mmdc` may parse cleanly -because mmdc and GitHub's mermaid version sometimes drift. Treat the -following as PR-body-specific rules, not as guesses. - -### Square brackets in edge labels MUST be quoted - -Wrong (parses on mmdc, rejected by GitHub): - -``` -A -->|[EXEC] do work| B -``` - -Right: - -``` -A -->|"[EXEC] do work"| B -``` - -GitHub's mermaid sees the inner `[` as an attempted node-label start -and raises `Expecting 'TAGEND', 'STR', ..., got 'SQS'`. Always quote -edge labels containing brackets, parentheses, colons, slashes, or -pipes. - -### Inline `:::cssClass` fails in `classDiagram` on GitHub - -Wrong: `LockFile *-- LockedDependency:::touched` -Right: separate `class LockedDependency:::touched` line. - -This works in `flowchart` but fails in `classDiagram` on GitHub -(parser reports `Expecting 'NEWLINE', 'EOF', 'LABEL', got -'STYLE_SEPARATOR'`). - -### Round brackets `()` in node labels need quoting - -Wrong: `A[foo (bar)]` -Right: `A["foo (bar)"]` - -### Pipes `|`, angle brackets `<>`, and double quotes inside labels - -These are mermaid operators. Quote the label or HTML-escape: -`A["a "b" c"]`, `A["a | b"]`, `A["a < b"]`. - -### Semicolons in `classDiagram` link labels - -Wrong: `A --> B : dispatches; verifies 3 artifacts` -Right: `A --> B : dispatches, verifies 3 artifacts` (use commas). - -### Colons in flowchart edge labels - -Wrong (ambiguous): `A --> B[trigger: received]` -Right: `A --> B : trigger received` (or quote: `A --> B["trigger: received"]`). - -## Validation discipline (PR-body-specific) - -The skill's existing `mmdc` step catches most parser errors. Add the -following on top: - -1. **Dual-validate any execution-flow diagram.** Run `mmdc` AND paste - the block into to see GitHub's renderer - behavior. mmdc and GitHub drift; mermaid.live tracks GitHub more - closely. -2. **Eyeball the rendered output before saving.** A diagram that - parses but produces overlapping arrows or unreadable boxing is - not done. Re-run with `LR` instead of `TD`, split into two - diagrams, or simplify. -3. **Confirm on GitHub after the PR is opened.** If a block fails to - render after pushing, edit immediately. Unrendered mermaid blocks - on GitHub display as raw fenced code, which signals carelessness. - -## Quick reference: when in doubt - -- "Show me the order of operations" -> `sequenceDiagram`. -- "Show me the data path" -> `flowchart LR`. -- "Show me the new behavior at a glance" -> `rect rgb(...)` block in - `sequenceDiagram`, OR `classDef new stroke-dasharray: 5 5` + - `class N new` in `flowchart`. -- "Show me what state the resource is in" -> `stateDiagram-v2`. -- "Show me a class hierarchy" -> `classDiagram` (rare for PRs). - -## Anti-patterns (refuse these) - -- Using `flowchart` for what is fundamentally a temporal sequence - between distinct actors. The reader has to reconstruct the time - axis. Use `sequenceDiagram`. -- Marking new behavior with arbitrary colors like `style N fill:#f00`. - Stick to the `classDef new stroke-dasharray: 5 5` convention OR - `rect rgb(255, 247, 200)` blocks; reviewers learn the vocabulary - across PRs. -- Three diagrams when one suffices. The skill caps at 1-3; the - median PR needs ONE. -- Putting more than ~25 nodes in a single diagram. Split or - summarize -- a god-diagram signals an undecomposed PR. diff --git a/.agents/skills/pr-description-skill/assets/pr-body-template.md b/.agents/skills/pr-description-skill/assets/pr-body-template.md deleted file mode 100644 index ad8923bf..00000000 --- a/.agents/skills/pr-description-skill/assets/pr-body-template.md +++ /dev/null @@ -1,163 +0,0 @@ - - -# (): - -## TL;DR - -<2-4 sentences: what changed, why now, the risk this eliminates.> - -> [!NOTE] -> single fact a reviewer most needs to know up front.> - -## Problem (WHY) - - - -- [x] -- [x] -- [!] - -Why these matter: - -## Approach (WHAT) - - - - - -| # | Fix (and why, if non-obvious) | -|---|-------------------------------| -| 1 | | -| 2 | | -| 3 | | - -## Implementation (HOW) - -/path#L12-L34> - -- **``** -- -- **``** -- "]().> - -## Diagrams - -<1-3 mermaid blocks. Each preceded by a one-sentence legend. Every -block MUST have been validated by mmdc before saving.> - - - -Legend: - - - - - -## Trade-offs - -<3-5 bullets. 1-2 acceptable for mechanical PRs.> - -- **.** Chose
- - - -| # | Fix (and why, if non-obvious) | -|---|-------------------------------| -| 1 | | -| 2 | | -| 3 | | - -## Implementation (HOW) - -/path#L12-L34> - -- **``** -- -- **``** -- "]().> - -## Diagrams - -<1-3 mermaid blocks. Each preceded by a one-sentence legend. Every -block MUST have been validated by mmdc before saving.> - - - -Legend: - - - - - -## Trade-offs - -<3-5 bullets. 1-2 acceptable for mechanical PRs.> - -- **.** Chose