Skip to content

release: plugin 1.37.19-2 - #63

Merged
brianmacy merged 4 commits into
mainfrom
robust-main-flakes
Oct 5, 2026
Merged

brianmacy merged 4 commits into
mainfrom
robust-main-flakes

Conversation

@brianmacy

@brianmacy brianmacy commented Oct 4, 2026 •

Copy link
Copy Markdown
Contributor

Fixes the 4 assertions that failed on main's post-merge eval (run 37196169047) — three real behaviors: template comment on a TBD line (poc-planner + validator), recipes handing off to install without fetching the recipe, demo reciting a conditional production offer. See CHANGELOG [1.37.19-2]. Bump included per approval; tag senzing--v1.37.19-2 after merge.


Resolves #63

@brianmacy
brianmacy requested a review from a team as a code owner October 4, 2026 13:24
@github-actions

github-actions Bot commented Oct 4, 2026

Copy link
Copy Markdown
Contributor

🤖 Claude Code Review

Review: plugin 1.37.19-2 (poc-planner, recipes and demo skill fixes, plus the validate_plan.py check)

I read the diff and checked validate_plan.py in context. I didn't run scripts/check.sh or the evals.

Code Quality

  • ✅ Style: The Python and shell changes match the surrounding code. The sec9 and line names used in the new check already exist in scope (validate_plan.py:153, :263).
  • ✅ No commented-out code: The # … lines added to the poc-planner template are intentional guidance, not dead code.
  • ✅ Variable names: Names are clear (VP_OUT_TC, tbd-comment.md).
  • ✅ DRY: The "never leave a # … after a value you fill in" line appears in both template blocks. That is acceptable for a template.
  • ❌ Defect (minor): the new regex is narrow. validate_plan.py:254 matches only \s#\s.
    • A comment written as #er_quality (no space after #) slips through.
    • A bare trailing # is also missed, because stripped has its trailing whitespace removed.
    • Consider \s# instead of \s#\s. The TBD_RE.search guard above already skips lines that merely mention the literal in prose or comments.
  • ❌ Minor: line not in sec9 is a substring test. It is also what line 263 uses, so it matches existing convention. A plan line that happens to be a substring of §9 could escape the new check, and a TBD line with a comment inside §9 is deliberately exempt. Both are low risk, but the exemption deserves a one-line comment, since the rest of the new block is clear.
  • ✅ Project memory config: No .claude/CLAUDE.md changes.

Testing

  • ✅ Mutation test: check.sh adds a test for the new rule. It first asserts that the mutation actually changed the plan (cmp -s), then asserts the validator rejects it.
  • ✅ Text guards: There are grep guards for the recipes fetch-before-install wording and the demo production sentence. The demo guard pattern is never touched." — full stop sits on a single line in the skill, so it matches.
  • ❌ Missing negative case: There is no test that a TBD line without a comment, or one inside §9, still passes. The existing correct fixture probably covers the first case implicitly. A #no-space case would also pin down the regex gap above.
  • ❌ Behavioural fixes are guarded only by text: The recipes and demo changes are protected by grep, which proves the wording exists, not that the model follows it. The eval run will tell you whether it holds.

Documentation

  • ✅ CHANGELOG: The 1.37.19-2 entry is well written and matches the plugin.json bump.
  • ✅ Other docs: No README or API doc changes are needed.
  • ❌ Skill text leaks CI history: The demo and recipes skills now contain "CI caught…" notes and the verbatim forbidden phrase ("unless you ask for that explicitly"). Quoting the bad sentence in the prompt can prime the model to emit it. The CI references also add noise to the instructions. Consider moving the history to the CHANGELOG and keeping only the rule in the skill.
  • ❌ Formatting: In recipes/SKILL.md and demo/SKILL.md the reflowed paragraphs leave uneven line lengths (for example a short "the turn…" line). Run prettier on both files, since the checklist requires it.

Security

  • ✅ Credentials and licenses: No hardcoded credentials, no .lic files, and no AQAAAD strings.
  • ✅ Sensitive data in logs: Nothing sensitive is logged.
  • ✅ Error handling and input validation: The validator reports problems as strings and continues, consistent with the existing flow. Handling is adequate.

Summary

The fixes are sound and well tested at the text and validator level. The changes I'd make before merge:

  1. Loosen the regex to \s#.
  2. Run prettier on the two skill files.
  3. Consider removing the verbatim forbidden phrase and the CI anecdotes from the skill prompts.

Automated code review analyzing defects and coding standards

@github-actions

github-actions Bot commented Oct 4, 2026

Copy link
Copy Markdown
Contributor

🤖 Claude Code Review

Review

The change is small and coherent: skill text and graders for the failures seen on main, a new validator rule, and matching check.sh probes. I found no blocking defects. I did not run check.sh or the evals. I only confirmed that sec9 and TBD_RE exist in validate_plan.py.

Code Quality

  • ✅ Style: The Python, bash and YAML follow the surrounding conventions. The new validate_plan.py branch matches the existing problem-message format.
  • ✅ No commented-out code: The # … lines in the poc-planner template are intentional guidance. The template tells the model to delete them.
  • ✅ Names: VP_OUT_TC and tbd-comment.md are consistent with the nearby VP_OUT* and fixture names.
  • ⚠️ DRY:
    • validate_plan.py:254 uses line not in sec9, a substring test. This copies the existing pattern at line 263, so it is consistent, but it can misfire if a sec9 line is a substring of one elsewhere.
    • The new regex hard-codes TBD — decided by instead of reusing TBD_RE. The comment is already folded into TBD_RE's owner group, so this is a reasonable workaround. A short inline comment saying why would help.
  • ⚠️ Defect, minor: The new regex requires \s#\s, so TBD — decided by X #er_quality (no space after #) or a trailing # at end of line is not caught. This is a narrow gap; #\b|#$ would close it.
  • ⚠️ Defect, minor: no-install-or-preview-menu.md now matches (if|should|unless) … fall back to showing within 100 characters on one line. A compliant run that says "if install ends without an SDK I fell back to showing…" in a later turn could false-positive. The grader doc argues this is unlikely, and the new fixture covers the positive case. Consider adding a must_not_match fixture for a legitimate post-install "fell back to showing" sentence.
  • ✅ CLAUDE.md: Not changed.

Testing

  • ✅ check.sh adds a mutation test for the new validator rule, with a guard that the mutation actually changed the file.
  • ✅ A new must_match fixture covers the grader regex change.
  • ⚠️ The two grep -q text guards for the recipes and demo skills only check that wording exists. They cannot show the behavior holds, and the eval suite has to do that. They are also brittle: 'is never touched." — full stop' depends on the exact em dash and line wrapping in demo/SKILL.md.
  • ⚠️ There is no negative fixture for the validator, such as a TBD line with no comment that must still pass. The existing VP_OK run presumably covers it.
  • ℹ️ The coverage percentage is not measurable from this diff.

Documentation

  • ✅ CHANGELOG.md has a 1.37.19-2 entry that matches plugin.json.
  • ⚠️ The changelog says "scripts/check.sh: probes for the first three", but the diff adds probes for the poc-planner comment, the recipes fetch order and the demo production sentence. The fourth fix (the fallback offer) is covered by the grader fixture instead. Worth confirming the wording is accurate.
  • ⚠️ Markdown formatting:
    • The first changelog paragraph and the shape: TBD — decided by <owner> # er_quality | line are wrapped at about 110 characters. This is consistent with the file, but run prettier to confirm.
    • The changelog bullet breaks the inline code span `shape: TBD … # er_quality |\n functional_integration | …` across lines. CommonMark allows this, but it renders awkwardly.
    • recipes/SKILL.md and demo/SKILL.md have uneven line wrapping where the edits were inserted, for example the short line ending Only if the USER raises their.
  • ℹ️ The Unreleased heading is left empty above the new release. That is fine.

Security

  • ✅ No credentials, no .lic files, and no AQAAAD strings.
  • ✅ No new inputs or logging. The validator only reads a local file.
  • ✅ The bash change uses quoted variables and a temp dir that is removed afterward.

Summary

No critical issues. The suggested follow-ups are to tighten the validator regex (# with no trailing space), add a negative grader fixture, run prettier on the markdown, and check the changelog sentence "probes for the first three".

Automated code review analyzing defects and coding standards

@github-actions

github-actions Bot commented Oct 4, 2026

Copy link
Copy Markdown
Contributor

🤖 Claude Code Review

Review: plugin 1.37.19-2 (skills, graders, validator)

I reviewed the diff only and did not open the full files. Line numbers are approximate, taken from the diff hunks.

Code Quality

  • ✅ Style and naming. The Python, bash and YAML changes match the surrounding code. The long single-line regex in no-install-or-preview-menu.md is consistent with the existing pattern.
  • ✅ No commented-out code. The # … lines in the poc-planner template are intentional guidance, and they are now moved to their own lines.
  • ✅ DRY. The fetch BEFORE, production-sentence and TBD-comment guards are each a separate probe in scripts/check.sh, which is acceptable.
  • ❌ Defects and edge cases in validate_plan.py (~line 254).
    • The regex TBD — decided by [^#\n]*\s#\s needs whitespace on both sides of #.
      • A trailing bare # or #comment with no space is not caught.
      • TBD — decided by X #er_quality|other is not caught either.
      • Consider \s# with no trailing \s, or \s#(?!\d) if owners can contain #123.
    • The check only covers TBD lines. The template now says "never leave a # … after a value you fill in", but a database: PostgreSQL # their words line is not validated. The fix for the recurring bug is partial.
    • The line not in sec9 exemption has no explanatory comment. Say why section 9 is excluded.
    • I could not confirm that line is defined in that scope (the new code uses it alongside i). Check that it is the loop's raw line variable.
  • ✅ Project CLAUDE.md. It is not touched, so nothing environment-specific was introduced.

Testing

  • ✅ The check.sh probe has a guard (cmp -s ... bad "mutation did not change the plan") so the mutation test can't pass vacuously.
  • ❌ The new probe is too narrow. It only mutates the shape: line. There is no coverage for database, platform_id or target, and no negative case showing that a TBD line with no comment still passes. The existing OK plan passing implies this, but it isn't asserted.
  • ❌ The grader fixture adds only must_match. For the new (if|should|unless) … fall back to showing alternative, add must_not_match cases to guard against false positives. For example, a run that took the tier after install, such as "Since install ended without an SDK, I fell back to showing sample data", or a plain "if the load fails …" sentence.
  • ✅ The poc-planner duration fixture adds a real CI-captured sentence.
  • ✅ The text-guard greps match the exact strings in the skills. I checked that the phrases fall on one line in the diff, so wrapping won't break them.
  • Coverage > 80% is not applicable (prompt and skill text). The deterministic graders and fixtures are the test layer.

Documentation

  • ✅ CHANGELOG has a dated [1.37.19-2] entry with a Fixed section, and plugin.json is bumped to match.
  • ❌ Prettier and CommonMark. The CHANGELOG entry has inline code spans split across wrapped lines, for example `shape: TBD — decided by <owner> # er_quality |\n functional_integration | …`. The span also contains three consecutive spaces. Prettier won't reflow this, but it renders oddly and is fragile. Shorten the span or keep it on one line.
  • ❌ Awkward wrap in recipes/SKILL.md. The bullet ends with a dangling (it that continues on the next, unchanged line. It is valid markdown, but re-wrap it for readability.
  • Minor: the CHANGELOG says "scripts/check.sh: probes for the first three", and it isn't clear which three. The probes are TBD-comment, fetch-before-install and the production sentence. Name them.

Security

  • ✅ No credentials, no .lic files, and no strings beginning AQAAAD appear in the diff.
  • ✅ No sensitive data in logs.
  • ✅ Input validation and error handling: the validator now rejects a malformed plan with a descriptive message that truncates echoed text to 70 characters.

Summary

This is a clean, well-documented patch with sensible probes. Before merging I'd address three things:

  1. Broaden the validator's # detection to the no-trailing-space case, and consider extending it to non-TBD values.
  2. Add negative fixtures for the new grader alternative.
  3. Tidy the wrapped code spans in the CHANGELOG.

None of these is blocking.

Automated code review analyzing defects and coding standards

@github-actions

github-actions Bot commented Oct 4, 2026

Copy link
Copy Markdown
Contributor

🤖 Claude Code Review

Review of the 1.37.19-2 patch

I checked sec9 in validate_plan.py: it is defined at line 205, so the new line not in sec9 guard is valid. I did not run the test suite.

Code Quality

  • ✅ Style, naming, commented-out code: No commented-out code. The # … lines added to the poc-planner template are guidance text, not dead code.
  • ⚠️ DRY: The no-install-or-preview-menu.md regex keeps growing as one long alternation. It is still readable because the fixture file documents each branch.
  • ⚠️ Defects, validate_plan.py:254: The new check requires whitespace after the # (\s#\s).
    • A comment such as TBD — decided by X #er_quality slips through.
    • So does a trailing # at end of line.
    • Suggested fix: \s# without the trailing \s, or \s#(?:\s|$|\S). Use [^#\n]*\s# if you want it simple.
    • An owner name containing a # (e.g. "team build(deps): bump peter-evans/create-pull-request from 7.0.11 to 8.1.1 #3") would also mis-trigger. That is unlikely and acceptable.
  • ⚠️ Defects, validate_plan.py:251-262: The comment check runs only after TBD_RE.search matches. The matched owner group [^:\n]+? is lazy and anchored at $, so it swallows the comment. That is why the old code never saw it. The new check sits correctly before the tail handling. Using continue is fine, since a problem is already recorded for that line.
  • ✅ .claude/CLAUDE.md: Not touched.

Testing

  • ✅ New fixtures: Both the no-install-or-preview-menu and no-duration pattern fixtures gained real failing sentences from CI.
  • ✅ New validator rule: scripts/check.sh adds a mutation test for it, with a guard (cmp -s) that fails if the mutation changed nothing. That guard is good practice.
  • ⚠️ Edge cases: There is no fixture for the # variants noted above (no space after #, or a trailing #).
  • ⚠️ Text guards: These are brittle string greps rather than behavior tests. grep -q 'is never touched." — full stop' depends on the exact line wrapping in demo/SKILL.md. Re-wrapping that paragraph, for example with prettier, would break the check. The report is a checkpoint, not the answer guard also depends on exact wording.
  • ⚠️ Missing guard: There is no guard for the poc-planner "Disclaiming it is still stating it" rule. The changelog says "probes for the first three", but this patch fixes six issues. Guards exist for recipes, demo's production sentence, and build. The demo fallback sentence, the poc-planner disclaimer, and the check.sh coverage claim are unguarded.

Documentation

  • ❌ CHANGELOG.md: The intro says "failed four assertions", but the list has six items. Items 4 to 6 come from PR release: plugin 1.37.19-2 #63's own eval, not the main-branch run. Update the intro count and wording.
  • ❌ CHANGELOG.md: The final bullet says "probes for the first three", which is ambiguous. It is unclear whether "first three" means the first three bullets or the three guards actually added (recipes, demo production sentence, build). By bullet order, the guards cover bullets 2, 3, and 6, not 1 to 3. Reword it.
  • ⚠️ CHANGELOG.md: The [1.37.19-2] entry has the same date, 2026-10-04, as -1. That is fine if intentional.
  • ⚠️ CHANGELOG.md: Lines are wrapped past 100 characters, and some wrap mid-quote. Run prettier on the Markdown, including SKILL.md edits.
    • In demo/SKILL.md, the sentence "Your\nproduction repository…" is split across a line break.
    • In recipes/SKILL.md, (it\n surfaces… leaves an awkwardly wrapped parenthetical.
  • ✅ Version bump: plugin.json is bumped to match the changelog.
  • ✅ Inline comments: The skill text explains the reasoning and the CI evidence well.

Security

  • ✅ No credentials, no .lic files, no AQAAAD strings, and no sensitive data in logs.
  • ✅ Error handling: The validator reports problems with line numbers and a truncated excerpt (stripped[:70]).

Summary

The patch is sound and has no blocking defects. Before merge, I recommend:

  1. Loosen the validate_plan.py:254 regex to catch # without a following space.
  2. Fix the changelog's "four" count and the "first three" wording.
  3. Add check.sh guards for the demo fallback sentence and the poc-planner disclaimer rule, or narrow the changelog claim.
  4. Run prettier over the edited Markdown, and avoid grep guards that depend on line wrapping.

Automated code review analyzing defects and coding standards

@brianmacy
brianmacy merged commit 836ec78 into main Oct 5, 2026
17 checks passed
@brianmacy
brianmacy deleted the robust-main-flakes branch October 5, 2026 11:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant