Add workflow_dispatch-only cmd.exe paren-nesting hazard probe (Item 61) - #461
Merged
Conversation
CLAUDE.md Active Backlog Item 61's remaining scope needs a real cmd.exe to settle whether a same-line (`(`/`)`) pair nested inside a real if-block is safe the same way a top-level one is, or whether nesting depth or a `>>` redirection prefix is the actual trigger -- static reasoning about this hazard class has been wrong three times already in this repo (docs/agent-lessons-learned.md). This sandbox has no Windows access at all, and the repo's existing CI triggers (push/pull_request) would run the full 8-lane matrix to answer a question needing a few seconds of real cmd.exe time. Adds a new, separate, workflow_dispatch-only workflow (matching the existing workflow-lint.yml pattern -- deliberately not wired to push/pull_request, so batch-check.yml's own triggers are untouched) whose one job runs a small fixture matrix (depth 1-4, with/without redirection) plus a positive control using the already-confirmed-broken cross-line shape, on a real windows-latest runner, and reports which fixtures survived. Reusable for any future instance of this same hazard class, not just Item 61. See docs/open-questions.md item 5 for the maintainer discussion this followed. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017SQ1rvJxDbE71pTXJ4QvLV
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Warning Review limit reached
Next review available in: 56 minutes Limit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?Wait for the limit to reset, then comment An organization admin can change what happens after included review limits in Billing. How do review limits work?CodeRabbit enforces per-developer PR review limits within each organization. For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Owner
Author
|
@coderabbitai review _Generated by Claude Code Generated by Claude Code |
mixmansoundude
enabled auto-merge (squash)
August 23, 2026 15:45
|
|
Owner
Author
|
@coderabbitai review Generated by Claude Code |
|
|
mixmansoundude
temporarily deployed
to
github-pages
August 23, 2026 17:49 — with 
GitHub Actions
Inactive
GitHub Actions
Inactive
mixmansoundude
added a commit
that referenced
this pull request
Aug 23, 2026
…l run (#463) PR #461 (the workflow_dispatch-only cmd.exe paren-nesting probe) merged to main. Tried to dispatch it via the GitHub Actions API and hit a new, narrower blocker: this session's GitHub integration returns 403 Resource not accessible by integration on the workflow_dispatch endpoint (confirmed against both repo-name casings), even though it can read checks and merge PRs. Rewords CLAUDE.md's Item 61 entry and docs/open-questions.md item 5 to reflect this actual current blocker (needs a manual run from the Actions UI, or the integration granted Actions-write permission) instead of the original "which unblocking mechanism" question, which the maintainer already answered. Claude-Session: https://claude.ai/code/session_017SQ1rvJxDbE71pTXJ4QvLV Co-authored-by: Claude <noreply@anthropic.com>
mixmansoundude
added a commit
that referenced
this pull request
Aug 24, 2026
…-line-paren question (#464) * Close Item 42's tag-classification precondition (lever 1) Classifies the 6 remaining :log tags lever 1's own INFO/BOOT/WARN/ERROR wording never named: STATUS, REPAIR, and HINT are visible-by-default (each is directly actionable or the run's own success/failure readout); INSTALL joins DEBUG/TRACE as suppressed-by-default (it sits strictly beneath the INFO-tier dependency-install progress line already shipped, and the file's own header comment at that call site already anticipated this classification). Also audits every test for a live-console-echo dependency on DEBUG/TRACE/ INSTALL before any tiering mechanism gets built: selfapps_pipgap.ps1 reads ~setup.log (untouched by tiering, not a blocker); selfapps_pvw_overrides.ps1 reads the console-redirected bootstrap log for a [DEBUG] line and would break the moment console suppression ships -- flagged as the one thing that must be fixed in the same change that implements lever 1's actual mechanism. Deliberately scoped to classification + audit only, not the tiering mechanism itself -- :log has 425 call sites, and this repo's own established discipline for a change at that blast radius is one careful slice at a time (see the DLL-bundling and hidden-import repair loops' own multi-slice history elsewhere in this backlog). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017SQ1rvJxDbE71pTXJ4QvLV * Close Item 61: same-line nested paren pairs are unsafe at any depth Real cmd.exe evidence (the paren-nesting hazard probe, PR #461, run manually by the maintainer after this session's GitHub integration hit a 403 trying to dispatch it itself) settles Item 61's last open question: a same-line, self-contained (/) pair nested inside a real if/for block corrupts cmd.exe's parsing at ANY nesting depth, with or without a >> redirection prefix -- even the shallowest case (one level, no redirect) failed identically to the known-broken control. check_delimiters.py's pop() no longer exempts a same-line close from the prose-paren hazard check -- only whether the pair is nested at all matters now, not whether it closes on the same or a later line. A related gap found while verifying against a real regression fixture: the echo-line detector never recognized a redirected form like '>> "%LOG%" echo ...' (the exact shape that broke in PR #445) as an echo line at all, so its own paren pair went untracked regardless of the same-line fix -- closed via a new ECHO_LINE_RE that matches an optional redirection clause before "echo". Two existing tests flipped from asserting "not flagged" to asserting "flagged" (their own comments already said this would happen once the checker caught up); one new test locks in the one shape that remains genuinely safe -- a plain top-level echo/rem with no enclosing block at all. Running the fixed checker against run_setup.bat surfaced 63 genuine, previously-invisible findings -- individually read in context and reworded to remove the literal parens, in batches, following this repo's established one-slice-at-a-time discipline for a change at this blast radius. Every changed line is a rem/echo line; no functional code or log-message content changed except one user-facing echo line reworded for clarity. docs/open-questions.md item 5 removed (fully answered). CLAUDE.md's Item 61 entry closed and moved to docs/agent-closed-backlog.md. docs/agent-lessons-learned.md's corresponding entry updated with the confirmed, final rule. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017SQ1rvJxDbE71pTXJ4QvLV * Address CodeRabbit review findings on PR #464 Two real findings, both fixed: 1. ECHO_LINE_RE missed the command-echo-suppressed "@echo" form, so a same-line nested paren pair on an "@echo" line would go untracked the same way the redirected-echo gap did before it was fixed. Now matches an optional leading "@". New regression test test_paren_pair_on_at_echo_line_nested_is_flagged. 2. Real pre-existing bug (not introduced by this PR, but in the diff's review scope): the warnfix-triggered PyInstaller rebuild's two failure branches set HP_BOOTSTRAP_STATE=error but never cleared HP_FRESH_BUILD_OK, so :write_fast_hash would still pair the CURRENT sources with whatever stale, warnfix-incomplete EXE is left in dist\ from before the failed rebuild -- the next run's fast path would then wrongly trust it as fresh and skip retrying the repair. Mirrors the identical PR #460 fix already applied to the ORIGINAL build's own failure branches. Unlike a DLL-bundle/hidden-import repair loop failure (bundling-only, does not need this per docs/agent-interconnect.md), a failed warnfix rebuild means the current EXE genuinely lacks a needed dependency, so the flag must be cleared here too. New static harness check batch.warnfix.fresh_build_ok_clear guards both branches, scoped to :run_entry_smoke's own body so it cannot pass on unrelated text elsewhere. Deliberately did NOT also delete the stored fast-check hash file (as CodeRabbit's own suggested diff did) -- the content-hash comparison already handles the "sources changed" case correctly regardless, and unconditionally deleting it would force an unnecessary rebuild on the next run even when the existing dist\ EXE is still genuinely fine (a transient warnfix-rebuild failure with unchanged sources). Clearing HP_FRESH_BUILD_OK alone is the precise fix. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017SQ1rvJxDbE71pTXJ4QvLV * Fix batch.req005.warn_gate regression from Item 61's pipreqs-WARN reword The Item 61 paren-hazard fix reworded run_setup.bat's pipreqs auto-detect WARN from "...auto-detected (pipreqs)" to "...auto-detected via pipreqs" (the nested same-line parens were a real hazard per the newly-confirmed cmd.exe rule). tests/harness.ps1's batch.req005.warn_gate check still required the old literal string, so it failed on every CI lane -- caught via 4 non-gating-lane CI failures on the same commit. Updated the check's expected pattern, plus the doc/test references to the old wording that were purely cosmetic (a demo-output sample and a comment/assertion string in a negative-match test that would have passed either way). * Register batch.warnfix.fresh_build_ok_clear in the NDJSON row registry The CodeRabbit-requested HP_FRESH_BUILD_OK fix (commit 986e33f) added a new static harness.ps1 check emitting this row id, but per CLAUDE.md's AGENT DIRECTIVE it was never added to docs/agent-ndjson.md's registry -- caught by the ndjson-registry-check advisory CI job. python tools/ check_ndjson_registry.py now reports a clean PASS (328/328 IDs matched). * Fix false positive: nested prose paren at top level wrongly flagged CodeRabbit's review of PR #464 found a real bug in check_delimiters.py's Item 61 fix: the "already nested" hazard test was bool(self.stack), true the moment ANY bracket is open -- including a prior prose paren from the SAME echo/rem line's own text, not just a genuine enclosing if/for block. Reproduced directly: `echo outer (inner (detail))` at true top level (no enclosing block anywhere) wrongly flagged its own second paren. Fixed by adding a per-line `is_prose` fact to StackItem (independent of stack state) and basing the hazard verdict on whether a genuine structural (non-prose) bracket is already open, not on stack non-emptiness. Verified against the reported false positive (now clean) and both existing true-positive shapes (same-line and cross-line pairs genuinely nested inside a real if(...) block -- still correctly flagged). No live instance of this shape existed in run_setup.bat itself (clean before and after), so this closes a latent risk for future edits. --------- Co-authored-by: Claude <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
CLAUDE.md's Active Backlog Item 61 has a remaining open question (also
docs/open-questions.mditem 5): does a same-line, self-contained(/)pair nested inside a realif (...)block corrupt cmd.exe's parsing the way a cross-line pair already does -- and if so, is nesting depth or the>>file-redirection prefix the actual trigger? Static reasoning about this exact hazard class has been wrong three separate times already in this repo (docs/agent-lessons-learned.md's "A literal(/)insideechotext..." entry) -- only a genuine cmd.exe run counts as evidence.This sandbox has no Windows access at all, and the repo's existing CI triggers (
push/pull_requestonbatch-check.yml) would run the full 8-lane matrix (conda/uv installs, PyInstaller builds, the whole self-test suite -- historically 50-90+ minutes) to answer something needing a few seconds of real cmd.exe time..github/workflows/batch-paren-hazard-probe.yml,workflow_dispatch-only (matching the existingworkflow-lint.ymlprecedent) -- deliberately not wired topush/pull_request, sobatch-check.yml's own triggers are untouched.tools/probe_paren_hazard.ps1: generates a small fixture matrix (nesting depth 1-4, with/without a>>redirection prefix on the paren-bearing line) plus a positive control using the already-confirmed-broken cross-line shape from PR fix: backlog items 8, 14, 19 -- UNC WARN, misleading syntax error, cache-lane trap #408/Item 52: distinguish pyproj_deps.py's genuine errors from its exit-1 not-found case #445, executes each via a realcmd.exe, and reports which fixtures "survived" (printed their own trailing marker, proving cmd.exe parsed the whole block correctly). The control exists so a clean same-line matrix can actually be trusted -- if the control doesn't show corruption, something about the harness itself is wrong, not cmd.exe.Verification
python3 -m yamllint/actionlint -onelineon the new workflow file: clean.python3 tools/check_delimiters.py tools/probe_paren_hazard.ps1: clean.tools/run_sanity_sweep.sh(compileall, pyflakes, delimiter check, CRLF check, markdownlint, yamllint, actionlint, ASCII sweep, PS parse sweep, pytest): all green, 560 passed / 3 skipped (unaffected by this change -- no product code touched).main, it can be dispatched on demand via the Actions UI (orworkflow_dispatchAPI) to actually answer Item 61's open question. This PR itself doesn't answer the question; it adds the tool to answer it.🤖 Generated with Claude Code
https://claude.ai/code/session_017SQ1rvJxDbE71pTXJ4QvLV
Generated by Claude Code