fix(planning): check-open-questions refuses duplicate headings, names its bound line, skips fences - #4065
fix(planning): check-open-questions refuses duplicate headings, names its bound line, skips fences#4065kyle-sexton wants to merge 1 commit into
Conversation
… its bound line, skips fences The gate graded the section under the first heading matching the register pattern and nothing checked for a second one. Since 0.39.5 the checklist template's own register carries unfenced example rows, so a ledger copied from the template with a live register appended below was graded on the example rows and reported a phantom status=open, and no message said which heading had been read. - heading_matches lists every matching heading with its line, skipping fenced blocks; more than one match exits 2 naming every line. - extract_section takes the matched line number instead of re-matching, and skips fenced blocks, so the body graded is the section named in stderr. The template's fenced bash block holds "# Step 3" comment lines that used to terminate the section and hide later rows. - Every register-derived ungradeable message carries the bound heading and line; the --brief path gets the same duplicate refusal and naming. - A fence opened and never closed is ungradeable, named as the cause. The row loop already skipped every row after such a fence, so an open row behind a stray fence graded clean. Exit-code contract unchanged. Cases 26 to 36 pin the new behaviour; planning 0.39.7. Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Claude finished @kyle-sexton's task in 6m 16s —— View job Code review complete
Reviewed
Everything else checked out: the |
|
Claude finished @kyle-sexton's task in 1m 6s —— View job Security review (
|
|
Last security-reviewed head: |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 93dab381d8
ℹ️ 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".
| /^#+[[:space:]]/ { | ||
| if (inside) { exit } | ||
| if (tolower($0) ~ pattern) { inside = 1; found = 1; next } | ||
| /^[[:space:]]*(```|~~~)/ { fenced = !fenced; next } |
There was a problem hiding this comment.
For valid nested-fence documentation, this parity toggle exposes headings that are still inside a code block. For example, a four-backtick fence may legitimately contain a three-backtick ledger example; the inner opening toggles fenced off, so an example ## Open-question register is counted as a second live heading and the gate exits 2. The same problem occurs when ~~~ appears inside a backtick fence. Track the opener's character and length, and only close it with a compatible delimiter.
Useful? React with 👍 / 👎.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| if [[ "$brief_named" -eq 1 ]]; then | ||
| deferred_section="$(extract_section 'deferred questions' "$brief")" | ||
| brief_awk=$? | ||
|
|
||
| if [[ "$brief_awk" -eq 3 ]]; then | ||
| brief_matches="$(heading_matches 'deferred questions' "$brief")" | ||
| brief_matches_status=$? | ||
| if [[ "$brief_matches_status" -eq 4 ]]; then | ||
| die_ungradeable "unterminated fenced block in: $brief (every heading after it is hidden; close the fence)" |
There was a problem hiding this comment.
This heading_matches call (and its exit-4 check) runs unconditionally whenever --brief is named, before anything downstream knows whether the cross-check is even needed. If the ledger's register has zero deferred/blocked rows, deferred_ids is empty and the cross-check is a no-op — the zero-match branch two lines below (if [[ -z "$brief_matches" ]]; then ... if [[ -n "$deferred_ids" ]]) already accounts for that by skipping the "no section" refusal when nothing needs checking. But the unterminated-fence refusal on line 298 has no equivalent guard: it fires as soon as the Brief has any unclosed fence anywhere in the document, even in a section unrelated to "Deferred questions", even when there are no deferred/blocked ids to verify.
In practice a Brief is often a large planning document with its own code samples. A single unrelated stray unclosed fence in some other section would flip an otherwise-clean run (nothing deferred, ledger fully resolved) to status=ungradeable, which contradicts this gate's own stated goal of a narrowly-scoped mechanical check. Consider deferring the heading_matches/unterminated-fence check on the Brief until it's known that deferred_ids is non-empty, matching the short-circuit that already exists for the "no such heading" case right below it.
No test in check-open-questions.test.sh currently exercises "ledger has no deferred/blocked rows" + "Brief has an unrelated unterminated fence" — the existing mixed fixture (used by tests 30/31/36) always has deferred/blocked rows, so this gap isn't caught.
| /^#+[[:space:]]/ { | ||
| if (inside) { exit } | ||
| if (tolower($0) ~ pattern) { inside = 1; found = 1; next } | ||
| /^[[:space:]]*(```|~~~)/ { fenced = !fenced; next } |
There was a problem hiding this comment.
Minor maintainability note: the fence-marker regex ^[[:space:]]*(\``|~~~)is now duplicated three times — here, inextract_section(line 92), and in the row loop (line 199, bash=~form). The comment above this function already flags thatheading_matchesandextract_section` "must agree on what a heading is"; a future change to fence detection (e.g. requiring matched fence length/char per GFM) has three call sites to update in lockstep, with nothing enforcing they stay in sync. Not blocking, but worth a shared constant or helper given how central "don't silently drift" is to this script's own design philosophy.
|
Claude has reviewed this PR 1 time. The lane skips further automatic reviews after 5; deleting this comment resets the count. |
1 participant
Closes #4073
Summary
check-open-questions.shgraded the section under the first heading matchingopen-question registerand nothing checked for a second one. Since planning 0.39.5 the checklist template's own register section carries unfenced example rows, so a ledger copied from the template with a live register appended below it was graded on the example rows and reported a phantomstatus=open. No message said which heading had been read. The template's register also holds a fencedbashblock whose# Step 3 ...comment lines were read as headings, so a live row written after that fence was invisible.Fix
heading_matcheshelper lists every heading matching the pattern with its line number, skipping fenced blocks. More than one match exits 2 naming every line and stating that the gate reads exactly one section.extract_sectionnow takes the matched line number instead of re-matching a pattern, and skips fenced blocks, so the body graded is by construction the section named in stderr.(register '<heading>' at line N). The--briefpath gets the same duplicate refusal and names the deferred-questions heading and line in its missing-id message.--helpstays current. Exit-code contract unchanged.Verification
bash plugins/planning/scripts/check-open-questions.test.sh: eight new cases (26 to 33) written first and observed failing on the unchanged script (11 assertions red), then green; three more (34 to 36) added for the unterminated-fence finding; 55/55 green.bash plugins/planning/tests/interview-defenses.test.sh: PASS=98 FAIL=0 (no skill prose touched).bash scripts/affected-tests.sh --run --explain: selects the co-located suite, passes; plugin.json and CHANGELOG are recorded no-suite.bash scripts/check-changelog-parity.sh --check-bump origin/main: clean.shellcheckon both shell files: clean.status=cleanwith and without--brief.Related
🤖 Generated with Claude Code