AIP-19: Build review-session-context skill for .dev-team-review.md - #264
jodavis-claude wants to merge 1 commit into
Conversation
Wrapper-tier component per the spec's Component Breakdown: compute/init scripts and sentinel-section read/write conventions mirroring use-context-file, scoped to the review worktree/clone root instead of a work-item-id-keyed file outside the repo. No dedicated pytest suite, per Wrapper tier (visual inspection sufficient) and the spec's own isolation-pattern note, which lists only resolve_pr_reference.py, decide_cleanup_action.py, gather-pr-evidence, and write-review-guide as needing a verification harness. Also bumps plugins/dev-team's plugin.json version (1.4.1 -> 1.4.2), required for any PR touching plugins/<name>/.
jodavis-claude
left a comment
There was a problem hiding this comment.
Reviewed AIP-19: Build review-session-context skill for .dev-team-review.md.
All three exit criteria are met:
- Computes and initializes
.dev-team-review.mdwith the documented frontmatter fields (repo_scope,isolation_kind,owner,repo,pr_number,pr_url,worktree_path,head_ref,guide_path,work_item_id), matching the spec's Interfaces section exactly. - Writing a named section uses the identical sentinel-section append/replace format documented by
use-context-file. - The three reserved section names for later deliverables (
Specialist Report,Quiz,Publish Log) are documented in the initial template and left unwritten.
Verified manually: fresh creation, idempotent second run, and the error path (unwritable parent exits non-zero via a caught OSError, no partial file left behind) all behave as documented. plugins/dev-team/.claude-plugin/plugin.json version was correctly bumped (1.4.1 -> 1.4.2) per this repo's CI convention for any PR touching plugins/<name>/.
No Priority 1-4 issues found (correctness/fault-tolerance, security, performance, documentation).
Minor (non-blocking) note: init-review-context.py/compute-review-context.py have no dedicated pytest suite, unlike the mirrored reference init-context-file.py/test_init_context_file.py. This follows the spec's explicit Wrapper-tier classification for this component (visual/manual inspection sufficient) rather than the Testable-tier pattern the reference happens to use, so I'm not blocking on it, but flagging since a future reviewer may want to weigh in if this component's scope grows.
Approving.
jodavis-claude
left a comment
There was a problem hiding this comment.
Sign-off review for AIP-19 (review-session-context skill).
Prior review threads: none existed — the first-pass review approved with no inline comments and zero open threads. Nothing to resolve here.
New commits since first-pass review: none. The branch is unchanged since that review (single commit c43be32), so there is nothing new to scan.
Re-verified: SKILL.md, compute-review-context.py, init-review-context.py, and review_context_template.md against the task brief's exit criteria, _spec_ReviewPrGuide.md's Interfaces section, CONTRIBUTING.md, and the mirrored reference (use-context-file). All three exit criteria remain met, the sentinel-section format matches, reserved section names (Specialist Report, Quiz, Publish Log) are documented and left unwritten, error handling in init-review-context.py is caught and non-silent, and plugin.json's version bump (1.4.1 -> 1.4.2) is present. No Priority 1-4 issues found.
Signing off.
Work item: AIP-19
AIP-19 required building
review-session-context: a new skill that computes, initializes, reads, and writes.dev-team-review.md— a small, worktree/clone-local state file for an in-progress PR review session — using the same sentinel-section append/replace conventions asuse-context-file, but independent of it.Changes
plugins/dev-team/skills/review-session-context/SKILL.md(new) — documents resolving, initializing, reading, and writing.dev-team-review.md: path resolution at<worktree-or-clone-root>/.dev-team-review.md(no repo-slug, no~/.dev-team/DEV_TEAM_STATE_DIR), the frontmatter field table, the same sentinel-section append/replace format asuse-context-file, the three reserved section names (Specialist Report,Quiz,Publish Log) documented but left unwritten, and a note that this skill is the file's sole writer.plugins/dev-team/skills/review-session-context/assets/review_context_template.md(new) — frontmatter template with the ten documented fields (repo_scope,isolation_kind,owner,repo,pr_number,pr_url,worktree_path,head_ref,guide_path,work_item_id), all blank, plus an HTML-comment block listing the three reserved section names for later deliverables.plugins/dev-team/skills/review-session-context/scripts/compute-review-context.py(new) — given a worktree/clone root, prints the absolute path to.dev-team-review.mdunder it. No git-remote/repo-slug logic, unlikeuse-context-file'scompute-context-file.py.plugins/dev-team/skills/review-session-context/scripts/init-review-context.py(new) — idempotent creation of the review context file from the template if it doesn't already exist; creates parent directories as needed; exits non-zero (leaving no partial file) on error.plugins/dev-team/.claude-plugin/plugin.json(modified) — bumpedversionfrom1.4.1to1.4.2, required for any PR touchingplugins/<name>/per this repo's CI convention.Design decisions
init-review-context.py/compute-review-context.py. The spec classifiesreview-session-contextas Wrapper tier (visual inspection sufficient, no dedicated unit test required by default) and its own isolation-pattern note lists onlyresolve_pr_reference.py,decide_cleanup_action.py,gather-pr-evidence, andwrite-review-guideas needing a verification harness. Verified the scripts manually instead: fresh creation, idempotency (second run is a byte-for-byte no-op), the error path (unwritable parent exits non-zero via caughtOSError, no partial file/directory left behind), and the sentinel append/replace format itself..featurefiles exist anywhere in this repo, and this component introduces no new externally-observable entry point of its own; the actual/dev-team:review-prcommand that will call it is a separate, later task (AIP-23).init-review-context.pytakes only the target<review-context-file>path as its argument, since (unlikeuse-context-file's template) none of.dev-team-review.md's ten frontmatter fields need substitution at init time — every field starts blank and is filled in later by whichever component resolves it.Testing completed
Manual verification of
init-review-context.pyandcompute-review-context.py: fresh creation (frontmatter fields present and blank, correct order), idempotency (second run is a byte-for-byte no-op), the error path (unwritable parent path exits non-zero, leaves no partial file/directory), and the sentinel section append/replace format (appended a new section, then replaced its content, both succeeding as documented).