Skip to content

🎞️ [just] gh-process v9.9 matured the PR/Copilot lifecycle and added opt-in asciinema recording - #51

Merged
chicks-net merged 1 commit into
mainfrom
chicks/2026-09-14-gh-process-9-9
Sep 14, 2026
Merged

chicks-net merged 1 commit into
mainfrom
chicks/2026-09-14-gh-process-9-9

Conversation

@chicks-net

Copy link
Copy Markdown
Owner

Done

  • 🎞️ [just] gh-process v9.9 matured the PR/Copilot lifecycle and added opt-in asciinema recording

Meta

(Automated in .just/gh-process.just.)

…opt-in asciinema recording

Signed-off-by: Christopher Hicks <chicks.net@gmail.com>
@claude

claude Bot commented Sep 14, 2026

Copy link
Copy Markdown

test comment from review

@claude

claude Bot commented Sep 14, 2026

Copy link
Copy Markdown

Review of #51 (gh-process v9.9)

This PR only touches the .just/ template-sync tooling (justfile recipes + shell libs), not main.go / the ctm CLI itself, so this review focuses on shell/justfile correctness and security rather than the Go conventions in CLAUDE.md.

Strengths

  • Real security fixes, well justified in comments:
    • branch/release in gh-process.just switch from {{ branchname }}/{{ rel_version }} interpolation to [positional-arguments] + "$1", and the release-version regex is now fully anchored (^v[0-9]+(.[0-9]+){0,2}$) instead of prefix-only, closing the quote-injection issue referenced as #359.
    • New validate_filepath/sanitize_for_display in .just/lib/common.sh, wired into template_update.sh and template-sync.just, guard against path traversal and control-character/quote injection from a compromised or malformed CHECKSUMS.json manifest (#347). The ..-segment check and control-char rejection look correct, and NUL-delimited iteration (jq -j / read -r -d '') correctly handles manifest keys with embedded newlines.
    • template_update.sh/template-sync.just jq calls switched from string-interpolated programs (building the jq program with $filepath inline) to --arg fp binding, removing a jq-injection vector.
    • checksums_diff's predictable /tmp/template-version-$$ path replaced with mktemp, removing a symlink/race TOCTOU risk.
    • The compliance_check repo-description check no longer aborts the whole recipe (under set -euo pipefail) when gh repo view fails; it now degrades to a yellow 'skipped' line.

Issues / things worth a second look

  1. Trailing whitespace introduced in .just/copilot.just. Many previously-blank lines (around the backup/diff-preview logic) now have trailing spaces. This looks like incidental noise rather than an intentional change, and is exactly what the new editorconfig_check recipe (added in this same PR) is meant to catch - worth cleaning up before merge so the new gate doesn't immediately start failing on this file.

  2. template_update.sh reintroduces a masked-return-code pattern. 'readonly MANIFEST_FILE; MANIFEST_FILE=$(mktemp)' (split into two statements, presumably to satisfy shellcheck's SC2155) was collapsed back into a single 'readonly MANIFEST_FILE=$(mktemp)'. That re-masks mktemp's exit status behind readonly's. Low practical risk since mktemp rarely fails, but if the split existed specifically to quiet shellcheck, this may reintroduce that warning - worth confirming CI's shellcheck run is still clean.

  3. generate_checksums.sh dedup check swapped a glob match for a regex match ([[ " $seen_checksums " =~ " $checksum " ]] instead of == ...). Behaviorally equivalent today since sha256 hex digests have no regex metacharacters, but there's no comment explaining the motivation, and using =~ where substring matching is intended reads as an accidental edit. Consider reverting to the glob form or adding a comment if the regex form is intentional.

  4. Executable bit flipped on several .just/*.just files (claude.just, compliance.just, cue-verify.just, gh-process.just, pr-hook.just, shellcheck.just, template-sync.just) and cue_sync.awk, none of which have shebangs or are ever invoked directly (they're parsed by just, not executed). Harmless, but likely an artifact of however this sync was generated/copied rather than an intentional change.

  5. release version regex is now stricter than before (^v[0-9]+(.[0-9]+){0,2}$), which is good for injection-safety but also now rejects pre-release/build-metadata tags like v1.2.3-rc1 or v1.2.3+build4 that the old prefix-only check would have allowed through. If those are ever used for releases here, this will need a follow-up; otherwise worth a one-line note that this narrowing is intentional.

  6. _again_inner now runs 'just _pr-hook' (shellcheck + the new editorconfig_check + claude_permissions_sort) before every git push via again. Functionally sound (guards against the #329 regression the comment describes), but every again invocation now pays for a full shellcheck + editorconfig pass - probably the right tradeoff for correctness, just flagging the added latency in case it becomes noticeable.

Test coverage

No new automated tests ship with this PR, but per the existing comments in cue-verify.just/cue_sync.awk, the .just/lib/*_test.sh harnesses only live in fini-net/template-repo itself and are stripped out of derived repos (like this one) by clean_template - so the absence of test changes here is consistent with how this repo consumes the template, not a gap introduced by this PR.

Overall: solid, well-commented hardening pass (path traversal, quote/shell injection, TOCTOU temp files). The trailing-whitespace regression in copilot.just (item 1) is the one thing worth fixing before merge given the new editorconfig gate; the rest are minor/FYI.

@chicks-net
chicks-net merged commit 5f934e2 into main Sep 14, 2026
11 checks passed
@chicks-net
chicks-net deleted the chicks/2026-09-14-gh-process-9-9 branch September 14, 2026 22:17
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