feat(launchpad): create the corpus-plan agent skill (#628) - #1561
Conversation
.claude/skills/corpus-plan/SKILL.md orchestrates manifest.py (#626) and issue_plan.py (#627): dry-run always before apply, report the four ApplyResult buckets by name (created/already_existed/ manual_actions/unresolved_blockers) rather than collapsing them, verify sub-issue links via link_sub_issue's own re-read rather than trusting the call not raising, and explain alias resolution (a manifest row's path, resolved to a live issue number only through apply's own ledger output). Names three concrete ambiguous-scope cases to stop and ask about rather than guess. No skill-specific validator exists in this repo yet, so verification is a frontmatter parse check plus the closest existing relevant suite (the corpus package's own tests, unaffected by this documentation-only change). Signed-off-by: Serina Mcfall <serina.mcfall@gmail.com>
tucktuck101
left a comment
There was a problem hiding this comment.
Review — corpus-plan agent skill (#628)
The skill's ordering (dry-run, explicit go-ahead, ledger back in on re-runs) is right, and its
honesty rules are the correct posture. But three of the code paths it hands the agent do not
work as written against the modules they call.
High — the Python snippet raises AttributeError when run verbatim
Step 1 calls manifest.build_manifest(plan) without binding the result; steps 2 and 3 then
pass manifest.rows (SKILL.md:40, :51) where manifest is the module.
build_manifest returns a Manifest dataclass (manifest.py:159) whose .rows is the
intended value, so the snippet is AttributeError: module 'manifest' has no attribute 'rows'.
Bind it: built = manifest.build_manifest(plan), then issue_plan.dry_run(built.rows).
High — a missing parent Feature does not surface in unresolved_blockers
SKILL.md:92-97 tells the agent that a parent_feature naming an issue that does not exist
"is apply's unresolved_blockers-shaped problem". apply never reads parent_alias at all:
its second loop calls only _apply_project_fields and _apply_blockers
(issue_plan.py:274-277), and _apply_blockers appends to unresolved_blockers solely for
members of issue.blockers. ApplyResult has no parent bucket.
An agent following this paragraph reports "the missing parent will show up in
unresolved_blockers", sees an empty list, and concludes the parent was fine — the silent-failure
mode the skill's own honesty rules exist to prevent. (Note: on the dependency branch, apply
does not link parents at all — see my review on #1560. Until that is wired, there is no code
path that detects this case.)
Medium — port and repo are free variables the skill never tells the agent to construct
This is not cosmetic. GitHubPort.__init__(project_owner=None, project_number=None) is the
default, and set_project_field returns False immediately when either is None
(issue_plan.py:126-129, :190-193). So a bare GitHubPort() pushes every Priority and
Effort of every row into manual_actions, which SKILL.md:65-66 then instructs the agent to
hand to a human verbatim — turning an automatable apply into a pile of manual work with no
signal that configuration was the cause.
Medium — step 5 gives no rule for resolving parent_number
link_sub_issue(port, repo, parent_number, child_number) is handed to the agent with no
statement of where parent_number comes from. The manifest carries parent_feature, a
free-form string copied verbatim into PlannedIssue.parent_alias, and the alias ledger maps
only document paths to numbers, so a Feature is never in it. The one step that establishes
parentage has no resolution rule, and the step never says it runs once per created row.
What is correct
- Dry-run-then-go-ahead, and passing the accumulated ledger back on re-runs, are exactly right
for an idempotent GitHub writer. - The instruction to read every proposed title/parent/blocker before asking for go-ahead —
rather than trusting a plausible dry run — is the kind of rule that actually prevents damage. - Frontmatter (
name,description) matches sibling skills in.claude/skills/. - Refusing to invent a parent from context is the right default.
- CI green at head (latest run per check).
Reviewed by tucktuck101's review lane. Both code claims above were checked against the
dependency branches at their current heads.
Summary
Adds
.claude/skills/corpus-plan/SKILL.md, which orchestrates the already-built
manifest.py(#626) andissue_plan.py(#627) into one safeprocedure: dry-run always before apply, honest reporting of all four
ApplyResultbuckets, and verified (not assumed) sub-issue linking.Related issue
Closes #628
Issue type
Task
Agent provenance
Objective
.claude/skills/corpus-plan/SKILL.md, the corpus-plan agent skill issue #628 asks for.Impacted components
.claude/skills/corpus-plan/SKILL.md
Note on this diff's base: built directly on
origin/launchpad, not ontask/626-corpus-manifestortask/627-corpus-issue-plan. A skill file isan instruction document that references those scripts by name/path — it
doesn't import them as Python, so it doesn't need them physically present in
its own branch to be a valid, mergeable artifact. This avoids stacking a
third branch on top of two already-open PRs.
Approach and rejected alternatives
Followed this repo's existing
.claude/skills/<name>/SKILL.mdconvention(frontmatter with
name/description/allowed-tools, then structuredmarkdown) -- modeled most directly on
review-final's andagentic-debugging's existing files for tone and section shape. The skill'sone hard rule (dry-run before apply, always) is stated up front rather than
buried, matching
agentic-debugging's "Core Principle" placement.Rejected: writing a Python CLI wrapper as part of this issue. Rejected
because #628's impacted components list only the skill file, and
issue_plan.py/manifest.pyalready expose exactly the functions(
dry_run,apply,link_sub_issue,build_manifest) a skill needs tocall directly -- a wrapper script would duplicate logic those modules
already own and tested.
Rejected: having the skill re-validate the one-document-one-task invariant.
Rejected because
manifest.build_manifestalready enforces it and raises onviolation (#626) -- re-checking it here would be the "re-doing narrow work"
review-final's own docs warn against, applied to planning instead ofreview.
Verification
No skill-specific validator exists in this repository. Verification is a
frontmatter parse check plus the closest existing relevant suite (the corpus
package's own tests, structurally unaffected by this documentation-only
addition, run to confirm nothing else in the corpus tree broke).
Command run:
Raw output:
Command run:
Raw output:
(The
FAILline is validate.py's own diagnostic output from a test thatdeliberately exercises a nonexistent-root path -- the suite's actual result
is
OK, 79/79, no failures, none of which this PR could plausibly affect.)Not verified
This skill has never been run by an agent against a real manifest and a
real
GitHubPort-- it is new instruction text, not new code, and this repohas no harness for dry-running a skill's instructions the way a unit test
dry-runs a function. Its first real exercise will be whoever next uses it to
plan actual document tasks. Did not verify the skill's prose against every
edge case
issue_plan.py's own test suite covers (e.g. the exact wording ofan unresolved-blocker reason) -- it references the result buckets by name,
not by exact string, deliberately, so it won't drift if
issue_plan.py'smessage text changes.
Security implications
None. A documentation-only addition; it changes no runtime behavior on its
own. It does instruct future use of write-capable GitHub operations
(
issue_plan.apply), which is why it makes dry-run-first the onenon-negotiable rule.
Escalations
None. #628's DoD items map cleanly onto
manifest.py/issue_plan.py'salready-built guarantees; no new judgment call was required to write this
skill beyond how to present those guarantees clearly.