Skip to content

feat(planning): draft-goal-condition skill — docs-conformant /goal shape + mechanical length gate - #592

Merged
kyle-sexton merged 4 commits into
mainfrom
feat/481-goal-prompt-crafting-skill
Jul 20, 2026
Merged

feat(planning): draft-goal-condition skill — docs-conformant /goal shape + mechanical length gate#592
kyle-sexton merged 4 commits into
mainfrom
feat/481-goal-prompt-crafting-skill

Conversation

@kyle-sexton

Copy link
Copy Markdown
Contributor

Summary

Adds a new planning skill, draft-goal-condition, that crafts a paste-ready /goal completion condition from a stated intent. It (1) reads the current official /goal docs live for the condition shape and character limit — nothing hardcoded, so the skill does not rot when the documented contract changes between Claude Code versions; (2) drafts a transcript-demonstrable condition in the doc's effective-condition shape; and (3) because a language model cannot reliably count characters, proves the draft fits the limit with a deterministic counter script rather than model estimation. A step-0 lever-fit gate routes interval-shaped work to /loop and cloud/sessionless work to routines//schedule instead of authoring a goal.

Placement: planning. This was the maintainer-confirmed placement in the #481 interview thread — the plugin's interview skill already locks a task contract (goal, constraints, acceptance criteria), which is nearly isomorphic to the /goal effective-condition shape; planning produces crafted artifacts from intent, exactly this skill's job. It ships as a standalone authoring helper (like questionnaire), not a new pipeline stage, so the README's "eight pipeline skills" framing is unchanged.

Skill name draft-goal-condition and the lever-fit gate scope addition are the maintainer's decisions recorded on #481.

Implementation

New files:

  • plugins/planning/skills/draft-goal-condition/SKILL.md — the skill (81 lines). Steps: lever-fit gate → read the live contract → draft to the effective-condition shape → mechanical length check + trim/re-validate loop → paste-ready output.
  • plugins/planning/scripts/goal-condition-length.sh — deterministic, model-free character-length gate. The limit is passed in by the caller (--limit N, read live from the docs), never baked into the script; reads the condition from stdin or --file. Counts Unicode code points (perl-primary, wc -m fallback). Exit 0 within limit, 1 over, 2 usage/env error; stdout chars=<n> limit=<N> status=<ok|over>.
  • plugins/planning/scripts/goal-condition-length.test.sh — companion black-box test (13 cases), auto-discovered by scripts/run-plugin-tests.sh.

Changed files:

  • plugins/planning/.claude-plugin/plugin.json — version 0.21.20.22.0 (minor, new feature); added keywords draft-goal-condition, goal, completion-condition, autonomous-goal.
  • plugins/planning/CHANGELOG.md0.22.0 entry.
  • plugins/planning/README.md — one skill-table row.

Frontmatter blast radius (called out for review): this is a brand-new SKILL.md, so all frontmatter fields are new:

  • name: draft-goal-condition
  • description: — 824/1536 chars. Triggers are single-quoted and /goal-specific ('craft a /goal', 'write a goal condition', 'set up an autonomous goal', 'make Claude keep working until X', 'my /goal is too long / over the limit', 'turn this into a completion condition'), plus explicit skip/route-elsewhere triggers for interval/cloud/one-shot work. No overlap with neighbors: interview locks a PLAN.md task contract; questionnaire produces a person-handoff doc; neither matches /goal authoring.
  • argument-hint: "[intent]", user-invocable: true, disable-model-invocation: false (auto-discoverable on /goal-crafting intent).

Verification

Current official character limit — verified live, not from training data. Fetched https://code.claude.com/docs/en/goal during implementation (2026-07-19). The page states verbatim: "The condition can be up to 4,000 characters." The limit is on characters, not tokens, and the evaluator "does not call tools, so it can only judge what Claude has already surfaced in the conversation" — hence the transcript-demonstrable requirement. The verified 4000 appears only in this PR body and the runtime doc; it is grep-clean of the skill and both scripts (grep -rn '4000\|4,000' plugins/planning/skills/draft-goal-condition plugins/planning/scripts/*.sh → no matches), satisfying the "no baked limit" acceptance criterion.

Length gate against a real conforming example (limit passed at runtime = the live-verified 4000):

$ printf '%s' 'All tests in test/auth pass and the lint step is clean: `npm test` exits 0 and `npm run lint` reports no errors. No file outside src/auth/ or test/auth/ is modified. Stop after 20 turns if not yet met.' | bash plugins/planning/scripts/goal-condition-length.sh --limit 4000
chars=201 limit=4000 status=ok
exit=0

Length gate against a real oversized example (same intent padded to 4,800 chars):

$ printf '%s' "$oversized" | bash plugins/planning/scripts/goal-condition-length.sh --limit 4000
chars=4800 limit=4000 status=over
exit=1

Suite / lint (all green locally):

  • bash plugins/planning/scripts/goal-condition-length.test.sh → 13/13 pass.
  • shellcheck --rcfile=.shellcheckrc + shfmt -d on both scripts → clean.
  • check-skill.sh draft-goal-condition → PASS, 0 errors (the single WARN is the literal Use when: phrasing preference; all planning neighbors — interview, questionnaire, brainstorm — use Use for/Use proactively and carry the same accepted WARN, so this matches the plugin-family convention; the regression-critical single-quoted-trigger check passes).
  • markdownlint-cli2 on the SKILL/README/CHANGELOG → 0 errors.
  • node scripts/validate-plugin-contracts.mjs and generate-catalog.mjs --check → pass / in sync.

Closes #481

Related

🤖 Generated with a Claude Code implementation subagent (issue #481)

…length gate

Craft a paste-ready /goal completion condition from a stated intent. The skill
reads the current official /goal docs live for the condition shape and character
limit (nothing hardcoded, so it does not rot when the documented contract changes
between Claude Code versions), drafts a transcript-demonstrable condition, and —
since a model cannot reliably count characters — proves the draft fits the limit
with a deterministic counter (scripts/goal-condition-length.sh, caller passes the
live limit; exit 0 within / 1 over / 2 usage) covered by a companion test. A
step-0 lever-fit gate routes interval-shaped work to /loop and cloud/sessionless
work to routines/schedule instead of authoring a goal.

Bumps planning 0.21.2 -> 0.22.0 (minor, new feature).

Closes #481

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011V31qpAHP3jfs76B9d5Rfo
@kyle-sexton kyle-sexton added the automated Opened by automation. label Jul 20, 2026
@claude

claude Bot commented Jul 20, 2026

Copy link
Copy Markdown

Claude finished @kyle-sexton's task in 3m 21s —— View job


PR Review: feat(planning): draft-goal-condition skill

  • Gather context and understand the request
  • Review changed files (SKILL.md, shell scripts, plugin.json, CHANGELOG.md, README.md)
  • Check shell script correctness and security
  • Verify test coverage adequacy
  • Check CLAUDE.md / plugin design rules alignment
  • Post review findings

Overall: High-quality implementation. The design decisions — live doc read, caller-supplied limit, deterministic counter, lever-fit gate — are all sound and well-executed. Three findings below; one is a correctness bug worth fixing before merge.


Finding 1 — Medium: $chars is never validated after counting; counter failure silently reports ok

goal-condition-length.sh lines 97–108

if command -v perl >/dev/null 2>&1; then
  chars="$(printf '%s' "$condition" | perl -CSAD -e 'my $c = do { local $/; <STDIN> }; print length $c;')"
else
  chars="$(printf '%s' "$condition" | wc -m | tr -d '[:space:]')"
fi

if [[ "$chars" -gt "$limit" ]]; then   # ← integer comparison

set -uo pipefail is used without -e. If perl exits non-zero or writes nothing, command substitution sets $chars to an empty string. Bash then evaluates [[ "" -gt "$limit" ]], emits an integer expression expected error to stderr, and treats the comparison as false — so execution falls through to:

echo "chars=$chars limit=$limit status=ok"
exit 0

This is a false positive: the script reports the condition as within the limit when the counter itself failed. No test exercises this path.

Fix: add a guard after the counting block:

if ! [[ "$chars" =~ ^[0-9]+$ ]]; then
  echo "error: character count failed (counter returned: '$chars')" >&2
  exit 2
fi

Fix this →


Finding 2 — Low: usage() hardcodes line range 2,20 — maintenance trap

goal-condition-length.sh line 27

usage() {
  sed -n '2,20p' "${BASH_SOURCE[0]}" | sed 's/^# \{0,1\}//'
}

The header comment currently ends at line 22 (# Output (stdout, greppable)...), so the 2,20 range already silently truncates the last two lines of the documented output format. Any future edit to the header will silently truncate usage output with no indication. The common alternative is to delimit with a sentinel (# END-USAGE or # ===) and sed to that:

usage() {
  sed -n '/^# Mechanical/,/^# Output/p' "${BASH_SOURCE[0]}" | sed 's/^# \{0,1\}//'
}

Or add the output-format line within the 2,20 range by collapsing the header to 20 lines.


Finding 3 — Informational: Skill's stdin invocation pattern is quoting-fragile for complex conditions

SKILL.md lines 58–61

The SKILL.md demonstrates:

printf '%s' "<drafted condition>" | bash "${CLAUDE_PLUGIN_ROOT}/scripts/goal-condition-length.sh" --limit <LIMIT_FROM_STEP_1>

A model constructing this command must shell-quote the drafted condition — any unescaped single quote, backtick, or $ in the condition text breaks the invocation or introduces unexpected expansion. The --file path (write condition to a temp file, pass --file) sidesteps quoting entirely and would be safer as the recommended primary invocation, with stdin as the secondary. This is a guidance gap, not a code defect.


Things that are correct and worth calling out

  • No baked limit anywhere. grep -rn '4000\|4,000' plugins/planning/skills/draft-goal-condition plugins/planning/scripts/ is clean. The limit flows caller → script exclusively.
  • set -uo pipefail catches undefined variable refs and pipeline failures. Appropriate for the control flow here (explicit exits everywhere).
  • [[ ! -f "$file" ]] path check correctly rejects directories and symlinks-to-missing-targets. cat -- "$file" correctly handles filenames starting with -.
  • printf '%s' "$condition" in the counting step avoids injecting a trailing newline before the pipe, consistent with the stripping done by command substitution.
  • ${CLAUDE_PLUGIN_ROOT} reference in SKILL.md Step 3 is the correct plugin-form-safe variable per the design rules.
  • Perl flags -CSAD: S(=I+O+E UTF-8) + A(ARGV UTF-8) + D(default PerlIO UTF-8 for all streams). All appropriate; length in this mode counts Unicode code points, not bytes.
  • Boundary test (case 3): "abcde" --limit 5 → exit 0 correctly implements "up to N characters" (inclusive) matching the documented contract.
  • Version bump 0.21.2 → 0.22.0 is correct semver for a new feature.
  • README "eight pipeline skills" framing is preserved: draft-goal-condition appears in the Stage column as "Standalone", consistent with questionnaire and setup, so the pipeline skill count is unchanged.

Windows core.filemode=false dropped the +x recorded locally; the repo
convention (and the exec-bit hygiene gate) requires shell scripts to be
mode 100755, matching the sibling scripts under plugins/*/scripts/.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011V31qpAHP3jfs76B9d5Rfo
@claude

claude Bot commented Jul 20, 2026

Copy link
Copy Markdown

Claude finished @kyle-sexton's task in 2m 44s —— View job


PR Review: feat(planning): draft-goal-condition skill

  • Gather context and understand the request
  • Read changed files (SKILL.md, shell scripts, plugin.json, CHANGELOG.md, README.md)
  • Verify status of previous review findings
  • Check new commit (927a6e7 — executable bit only, no code changes)
  • Post comprehensive review findings

The latest commit (927a6e7) adds only the +x executable bit to the two scripts — no code changes were made in response to the prior review. All three previous findings remain open.


Finding 1 — Medium (OPEN): $chars unvalidated after counting — counter failure silently reports ok

goal-condition-length.sh lines 97–108

if command -v perl >/dev/null 2>&1; then
  chars="$(printf '%s' "$condition" | perl -CSAD -e 'my $c = do { local $/; <STDIN> }; print length $c;')"
else
  chars="$(printf '%s' "$condition" | wc -m | tr -d '[:space:]')"
fi

if [[ "$chars" -gt "$limit" ]]; then   # ← integer comparison with potentially empty $chars

The script has set -uo pipefail but NOT set -e. If perl exits non-zero or produces no stdout, command substitution sets $chars to "". The script doesn't exit on assignment failure (no -e). Then [[ "" -gt "$limit" ]] emits integer expression expected to stderr, treats the comparison as false (bash exits the [[ with code 2, which the if reads as "not taken"), and execution falls through to:

echo "chars=$chars limit=$limit status=ok"
exit 0

Result: chars= limit=4000 status=ok + exit 0 — a false positive: the script reports the condition is within the limit when the counter itself crashed. No test covers this path.

Fix — add after the counting block and before the comparison:

if ! [[ "$chars" =~ ^[0-9]+$ ]]; then
  echo "error: character count failed (counter returned: '${chars:-<empty>}')" >&2
  exit 2
fi

And add a test case exercising the exit-2 path when the counter returns a non-integer (e.g., by mocking perl to exit non-zero or by calling the script in a subshell with PATH stripped of perl and patched with a broken wc).

Fix this →


Finding 2 — Low (OPEN): usage() range 2,20 already truncates the last two header lines

goal-condition-length.sh line 27

usage() {
  sed -n '2,20p' "${BASH_SOURCE[0]}" | sed 's/^# \{0,1\}//'
}

The header currently runs lines 2–22 (line 22: # Output (stdout, greppable): …). The range 2,20 already silently omits the output format line and the blank line before it. Any future header addition will silently truncate with no warning. The common fix is a sentinel-based range:

usage() {
  sed -n '/^# Mechanical/,/^# Output/p' "${BASH_SOURCE[0]}" | sed 's/^# \{0,1\}//'
}

Or just collapse the header to fit in 20 lines (the current content compresses easily to 18).


Finding 3 — Informational (OPEN): Stdin invocation pattern in SKILL.md is quoting-fragile for real condition text

SKILL.md lines 58–62

printf '%s' "<drafted condition>" | bash "${CLAUDE_PLUGIN_ROOT}/scripts/goal-condition-length.sh" --limit <LIMIT_FROM_STEP_1>

A model substituting a real condition containing a single quote, backtick, or $ into this template will produce a broken or unsafe invocation. The --file path (write condition to a temp file, pass --file) sidesteps quoting entirely — it should be the recommended primary invocation in the example, with stdin as secondary. The --file option is mentioned in passing on line 62 but not promoted to the primary example.


What is correct

  • No baked limit. The limit flows caller → script exclusively; grep -rn '4000\|4,000' plugins/planning/skills/draft-goal-condition plugins/planning/scripts/ is clean. This is the central design criterion and it's satisfied.
  • set -uo pipefail catches unset variable refs and pipeline failures in normal code paths.
  • [[ ! -f "$file" ]] check correctly rejects directories and symlinks to missing targets. cat -- "$file" handles filenames starting with -.
  • printf '%s' "$condition" in the counting step avoids injecting a trailing newline before the pipe, consistent with the stripping done by command substitution.
  • ${CLAUDE_PLUGIN_ROOT} reference in SKILL.md Step 3 is the correct plugin-form-safe variable.
  • Perl flags -CSAD are appropriate; length in this mode counts Unicode code points, matching the documented "characters" semantics.
  • Boundary test (case 3): "abcde" --limit 5 → exit 0 correctly implements "up to N characters" (inclusive).
  • Version bump 0.21.2 → 0.22.0 is correct semver for a new feature.
  • README "Standalone" placement preserves the eight pipeline skills framing; the new row matches existing table format exactly.
  • Lever-fit gate (Step 0) routing table is accurate and covers the documented non-/goal cases.
  • Doc-fetch failure handling in Step 1 is explicit: stop, cite the URL, offer the user two paths forward — never fall back to a hardcoded number.

Recommendation: Finding 1 is the only thing worth blocking on — it's a silent false positive in the gate that is the skill's main reliability guarantee. Findings 2 and 3 can ship with a follow-up if the team prefers.

Address three Claude-review findings on #592:

- Medium: goal-condition-length.sh ran with `set -uo pipefail` but not `set -e`,
  so a crashed counter (perl/wc failing or emitting nothing) left $chars empty;
  the `-gt` test then errored-and-fell-through to a false `status=ok`/exit 0.
  Add an integer guard on $chars right after counting — non-numeric now exits 2.
  Covered by a new test that shadows both perl and wc to force the failure path.
- Low: usage() used a fixed `sed -n '2,20p'` range that silently truncated the
  last two header lines. Switch to a sentinel range (/^# Mechanical/,/^# Output/)
  so usage stays in sync as the header changes.
- Info: SKILL.md now leads with the quoting-robust --file invocation (immune to a
  condition containing ', `, or $), keeping stdin as the simple-case secondary.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011V31qpAHP3jfs76B9d5Rfo
@claude

claude Bot commented Jul 20, 2026

Copy link
Copy Markdown

Claude finished @kyle-sexton's task in 1m 16s —— View job


PR Review: feat(planning): draft-goal-condition skill (re-review after afef2f0)

  • Gather context and understand the request
  • Read latest commit diff to verify prior findings
  • Check Finding 1 fix (counter failure guard)
  • Check Finding 2 (usage() range)
  • Check Finding 3 (SKILL.md quoting guidance)
  • Post updated review

All three findings from the prior reviews are resolved in afef2f0.


Finding 1 — Medium (FIXED)

goal-condition-length.sh lines 107–111

The integer guard is in place exactly as suggested:

if ! [[ "$chars" =~ ^[0-9]+$ ]]; then
  echo "error: character count failed (counter returned: '${chars:-<empty>}')" >&2
  exit 2
fi

The accompanying test (case 14) correctly shadows both counter backends by prepending $fake_bin to PATH with stub scripts that consume stdin and produce no output, forcing $chars to empty regardless of which branch the script takes — then asserting exit 2. The test design is sound.


Finding 2 — Low (FIXED)

goal-condition-length.sh lines 27–30

Sentinel-based range replaces the hardcoded 2,20:

sed -n '/^# Mechanical/,/^# Output/p' "${BASH_SOURCE[0]}" | sed 's/^# \{0,1\}//'

This correctly spans the entire header including the output-format line, and is resilient to future header edits.


Finding 3 — Informational (FIXED)

SKILL.md lines 55–62

The --file path is now the primary recommended invocation with an explicit explanation of why (immune to ', `, $ in the condition text), and stdin is correctly demoted to a secondary "simple case" note.


No remaining findings. This PR is ready to merge.

@kyle-sexton

Copy link
Copy Markdown
Contributor Author

Review-finding classification

All three Claude-review findings addressed in afef2f0d.

# Finding Severity Classification Action
1 goal-condition-length.sh has set -uo pipefail but not set -e; a crashed counter (perl/wc failing or emitting nothing) leaves $chars empty, the [[ "" -gt N ]] test errors-and-falls-through, and the script reports a false status=ok/exit 0 — the length gate silently passes when its own counter broke. Medium Valid — real reliability bug Fixed. Added an integer guard ([[ "$chars" =~ ^[0-9]+$ ]]) immediately after the counting block; a non-numeric count now exits 2 with a diagnostic. New test #14 shadows both perl and wc with empty-output stubs to force the failure path and asserts exit 2 (no false pass).
2 usage()'s sed -n '2,20p' silently truncated the header's last two lines (the header runs to line 22), so --help dropped the output-format line. Low Valid Fixed. Switched to a sentinel range sed -n '/^# Mechanical/,/^# Output/p' so usage output stays in sync no matter how the header grows/shrinks. Verified --help now prints through the final Output (stdout, greppable): ... line.
3 SKILL.md's primary invocation example piped the condition via printf ... | ... --limit, which is quoting-fragile if the condition contains ', `, or $. Informational Valid Fixed. Promoted the --file invocation (already supported) to the primary example, keeping stdin as a documented simple-case secondary.

Verification

  • goal-condition-length.test.sh → 14/14 pass (was 13; +1 counter-failure case).
  • shellcheck --rcfile=.shellcheckrc + shfmt -d on both scripts → clean.
  • markdownlint-cli2 on SKILL.md → 0 errors.
  • CI on afef2f0d: 15/15 checks green (plugin-gate, hygiene, contracts all pass).

@kyle-sexton
kyle-sexton marked this pull request as ready for review July 20, 2026 03:13
@claude

claude Bot commented Jul 20, 2026

Copy link
Copy Markdown

Claude finished @kyle-sexton's task in 2m 8s —— View job


PR Review: feat(planning): draft-goal-condition skill

  • Gather context and understand the request
  • Read changed files (SKILL.md, shell scripts, plugin.json, CHANGELOG.md, README.md)
  • Verify prior findings are resolved
  • Check for new issues
  • Post comprehensive review findings

This is a fresh read of all changed files in afef2f0. All three prior findings from the iterative review are confirmed fixed; no new issues found.


Prior Findings — All Fixed

Finding 1 — Medium (FIXED): $chars guard is in place at goal-condition-length.sh lines 108–111. A counter that crashes or emits nothing now exits 2 with a diagnostic rather than falling through to a false status=ok. Test case 14 shadows both perl and wc with empty-output stubs to exercise this path — the stub design is correct (stubs consume stdin, produce nothing, exit 0; $chars="" fails the integer guard; exit 2 asserted).

Finding 2 — Low (FIXED): usage() at line 29 now uses a sentinel range '/^# Mechanical/,/^# Output/p' instead of the hardcoded 2,20. The range correctly spans the entire header (lines 2–22) including the output-format line, and is resilient to future edits.

Finding 3 — Informational (FIXED): SKILL.md lines 56–62 now promotes --file as the primary invocation with an explicit rationale (immune to ', `, $ in condition text); stdin is correctly demoted to a secondary note.


New Pass — Nothing Found

A full independent read of the 6 changed files surfaced no new issues. Key confirmations:

  • Arg parsing edge cases are correct. --limit= (empty) is caught by the [[ -z "$limit" ]] guard; --limit --file (value is another flag) is caught by the ^[0-9]+$ regex; shift 2 || { ... exit 2 } fires when --limit is the last token. All correct.
  • Empty-file edge case is correct. If --file points to an empty file, cat produces nothing, command substitution yields "", and [[ -z "$condition" ]] fires with exit 2. ✅
  • wc -m fallback locale note is correctly documented. The test correctly skips the multibyte case (case 13) when perl is absent, because wc -m in a non-UTF-8 locale counts bytes. This is the right call — the limitation is acknowledged, not silently wrong.
  • No baked limit anywhere. grep -rn '4000\|4,000' plugins/planning/skills/draft-goal-condition plugins/planning/scripts/ remains clean. The limit flows exclusively caller → --limit → script.
  • README table placement correct. draft-goal-condition appears in stage "Goal authoring" with "Standalone" at the end of its description — eight pipeline skills are unchanged.
  • Semver bump 0.21.2 → 0.22.0 is correct for a new feature.
  • CHANGELOG entry follows Keep a Changelog 1.1.0 format correctly.
  • set -uo pipefail (no -e) is appropriate: the script uses explicit exits and || rc=$? capture patterns throughout; adding -e would fight the deliberate error-handling structure. The integer guard after counting is the correct substitute for the one place where a failed subshell would otherwise be silent.

No remaining findings. This PR is ready to merge.

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: afef2f0d4c

ℹ️ About Codex in GitHub

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

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

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

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

Comment thread plugins/planning/skills/draft-goal-condition/SKILL.md Outdated
The draft-goal-condition doc-fetch failure path guarded only the character
limit against drift, letting the condition shape silently fall back to the
embedded Step 2 four-part shape — contradicting the skill's own rule to never
bake the shape. The proceed path now requires the user to paste the live
condition shape as well as the limit, or defer until the docs are reachable.

Addresses the Codex P2 review finding on #592.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@claude

claude Bot commented Jul 20, 2026

Copy link
Copy Markdown

Claude finished @kyle-sexton's task in 1m 38s —— View job


PR Review: feat(planning): draft-goal-condition skill

  • Gather context and understand the request
  • Review latest commit (112b080) — doc-fetch failure fix
  • Read SKILL.md to verify live condition shape is now required on fetch failure
  • Check for any new issues introduced by the fix
  • Post comprehensive review findings

Codex P2 Finding — FIXED (112b080)

SKILL.md line 39

The Codex finding was correct: the prior doc-fetch failure path blocked on a missing character limit, but left the condition shape unguarded — Step 2 would silently fall back to the embedded four-part shape, violating the skill's own rule against baking in shape or limit. The fix addresses this precisely:

Before (afef2f0):

"Do not fall back to a remembered number… Offer the user two ways forward: paste the current character limit from that page so the counter (Step 3) can run…"

After (112b080):

"Do not fall back to a remembered number or shape — a stale limit or condition shape baked in here is precisely the drift this skill exists to avoid. Offer the user two ways forward: paste the current condition shape and character limit from that page — the shape drives the Step 2 draft, the limit drives the Step 3 counter — or defer until the docs are reachable. Never finalize a draft on a shape or limit that was not sourced live."

The fix is minimal, targeted, and correct. The failure path now symmetrically blocks on both the shape and the limit, matching Step 1's stated purpose of extracting both.


One Observation (Non-blocking)

Step 2 retains an embedded four-part shape description ("As of the contract this skill targets, that is: …"). This is acceptable — the "As of…" framing marks it as descriptive of what the live docs currently say, not as a hardcoded fallback. In the normal path (doc fetch succeeds), the model uses the live shape; in the failure path (doc fetch fails), the model is now blocked from proceeding without a user-supplied shape. The embedded description functions as a reading aid for future maintainers, not a secret fallback. No change needed.


Prior Findings — All Confirmed Still Fixed

Finding Commit Status
$chars guard — counter failure was a silent false positive afef2f0 FIXED — [[ "$chars" =~ ^[0-9]+$ ]] guard at line 108; test case 14 exercises both stub paths
usage() range 2,20 silently truncated header afef2f0 FIXED — sentinel range at line 29
SKILL.md stdin invocation was quoting-fragile afef2f0 FIXED — --file promoted to primary at lines 56–62
Doc-fetch failure path: shape not required from user (Codex P2) 112b080 FIXED — both shape and limit now required at line 39

No remaining findings. This PR is ready to merge.

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 112b0801e0

ℹ️ About Codex in GitHub

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

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

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

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

Comment thread plugins/planning/skills/draft-goal-condition/SKILL.md
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

automated Opened by automation.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

feat: /goal prompt-crafting skill — docs-conformant goal shape + mechanical length check

1 participant