#70: Add thin Beislið CLI and shared user-install foundation#74
#70: Add thin Beislið CLI and shared user-install foundation#74
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR extracts installer implementation from ChangesCLI and Installer Module Extraction
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
scripts/test_install.sh (1)
727-751: ⚡ Quick winAdd an assertion that update preserves the CLI target too.
_preserve_manifest_for_update()now restoresbin_dir, but this test only checks skill/hook targets and opt-ins. A regression here would silently move the CLI back to the default bin dir after update without failing the suite.Suggested assertions
assert_json_field "$STATE/install.json" skill_dirs.codex "$CODEX_SKILLS" assert_json_field "$STATE/install.json" hooks_dir "$HOOKS" + assert_json_field "$STATE/install.json" bin_dir "$BIN_DIR" + assert_json_field "$STATE/install.json" cli_path "$BIN_DIR/beislid" assert_symlink_to "$CLAUDE_SKILLS/verify" "$REPO_DIR/skills/verify" assert_symlink_to "$AGENTS_SKILLS/verify" "$REPO_DIR/skills/verify" assert_symlink_to "$CODEX_SKILLS/verify" "$REPO_DIR/skills/verify" assert_symlink_to "$HOOKS/credential_guard.py" "$REPO_DIR/hooks/credential_guard.py" + assert_symlink_to "$BIN_DIR/beislid" "$REPO_DIR/bin/beislid" assert_file_contents "$TMP/pi-update.log" "install $REPO_DIR"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/test_install.sh` around lines 727 - 751, The test test_update_preserves_manifest_opt_ins_and_targets needs to also assert the CLI target/bin dir is preserved by _preserve_manifest_for_update: add an assert_json_field check on the install manifest's "bin_dir" to match the expected BIN_DIR and add an assert_symlink_to that verifies the repository's bin/<cli_executable> points to that BIN_DIR target (use the same helpers assert_json_field and assert_symlink_to used elsewhere in the test).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/install_lib.sh`:
- Around line 362-365: The error text printed when uncommitted changes are
detected is entrypoint-specific; update the two echo messages that currently
reference "install.sh --update" to be entrypoint-neutral or mention both options
(for example: "re-run the installer with --update (e.g. ./install.sh --update or
beislid update)"). Locate the block using SCRIPT_DIR and the git status check
and replace the existing second echo (and any similar guidance) so users see
either both "install.sh --update" and "beislid update" or a neutral phrasing
like "re-run the installer with --update" to cover both entrypoints.
- Around line 271-286: The status check currently only validates skill symlinks
(loop over "$CLAUDE_SKILLS" "$AGENTS_SKILLS" "$CODEX_SKILLS") but does not
verify the new CLI or credential-hook links (e.g., the beislid CLI link or
enabled hook symlinks), so extend the same symlink/target verification pattern:
after the skills loop in the status function add checks that verify the beislid
CLI symlink (check that a "beislid" entry is a symlink and readlink matches the
expected target in "$SCRIPT_DIR"/bin or similar) and iterate enabled hook
entries (e.g., "$SCRIPT_DIR/hooks" or the variable that holds hook paths) to
ensure each is a symlink and points to the expected hook file; print
success/failure messages analogous to "✓ $skill"/"✗ $skill" and set failed=1 on
any mismatch so status returns nonzero when CLI or hook links are broken.
- Around line 247-266: The current inline python invocation (using python3 to
json.load the file referenced by MANIFEST and running subprocess.check_output)
can cause beislid status to fail if python3 is missing or the JSON is malformed;
update the block so it first verifies python3 is available and that MANIFEST is
readable, and make the Python snippet itself resilient by wrapping json.load and
subprocess.check_output in try/except and on any error printing safe fallback
values (e.g., manifest: unknown, installed_at: unknown, version: unknown,
installed_commit: unknown, current_commit: unknown) and exiting with success so
the script continues to the symlink checks; refer to MANIFEST, SCRIPT_DIR, the
python3 invocation, json.load, and subprocess.check_output to locate and harden
the code.
---
Nitpick comments:
In `@scripts/test_install.sh`:
- Around line 727-751: The test
test_update_preserves_manifest_opt_ins_and_targets needs to also assert the CLI
target/bin dir is preserved by _preserve_manifest_for_update: add an
assert_json_field check on the install manifest's "bin_dir" to match the
expected BIN_DIR and add an assert_symlink_to that verifies the repository's
bin/<cli_executable> points to that BIN_DIR target (use the same helpers
assert_json_field and assert_symlink_to used elsewhere in the test).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 1ddfb195-c8c0-44bf-82d4-266b4a6ef0e4
📒 Files selected for processing (7)
README.mdbin/beisliddocs/configuration.mddocs/how-to-use.mdinstall.shscripts/install_lib.shscripts/test_install.sh
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.beislid/workflow.md:
- Line 31: The current reply_command uses the POST
/repos/{owner}/{repo}/pulls/{number}/comments endpoint which creates new review
comments; either rename reply_command to something like create_comment_command
if you intend to keep creating top-level comments, or change reply_command to
target the replies endpoint POST
/repos/{owner}/{repo}/pulls/{number}/comments/{comment_id}/replies and include a
{comment_id} placeholder in the command string and JSON payload so existing
comments are replied to (update the reply_command value accordingly and ensure
any code that writes the JSON temp file supplies comment_id).
In `@scripts/test_install.sh`:
- Around line 126-146: The assert_json_missing helper currently treats any
Python error as “field missing”; update assert_json_missing so the embedded
Python code explicitly catches json.JSONDecodeError (print error to stderr and
exit with a distinct code, e.g. 2) and other exceptions (exit code 3), and
change the shell wrapper around the python3 call to check for those exit codes
and call note_fail with a clear message like "invalid JSON at $path" or "error
reading $path" instead of treating them as a successful missing-field check;
reference the assert_json_missing function and the embedded python3 here when
making the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: eb9eb269-e759-4085-bdea-128cc0e4f3e6
📒 Files selected for processing (3)
.beislid/workflow.mdscripts/install_lib.shscripts/test_install.sh
🚧 Files skipped from review as they are similar to previous changes (1)
- scripts/install_lib.sh
|
|
||
| ```beislid:pr_review_update | ||
| type: cli | ||
| reply_command: 'gh api repos/{owner}/{repo}/pulls/{number}/comments --method POST --input {json_file}' |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
What is the correct GitHub REST API endpoint to reply to an existing pull request review comment?
💡 Result:
To reply to an existing pull request review comment using the GitHub REST API, use: POST /repos/{owner}/{repo}/pulls/{pull_number}/comments/{comment_id}/replies Where comment_id is the ID of the top-level review comment you’re replying to (replies to replies aren’t supported). [1][2] Request body: include a JSON field body with the reply text. [1][2]
Citations:
🏁 Script executed:
cat -n .beislid/workflow.md | head -40Repository: sandsower/beislid
Length of output: 1971
Check GitHub API endpoint for reply_command—endpoint creates new comments, not replies.
Line 31 uses POST /repos/{owner}/{repo}/pulls/{number}/comments, which creates a new review comment on a pull request. However, the command name and line 21 context ("Reply bodies are written through JSON temp files") indicate the intent is to reply to existing review comments.
To reply to existing comments, the endpoint must be:
POST /repos/{owner}/{repo}/pulls/{number}/comments/{comment_id}/replies
Clarify whether the config should:
- Create new review comments (current endpoint is correct, but rename from
reply_commandfor clarity) - Reply to existing comments (update endpoint to include
{comment_id}/replies)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.beislid/workflow.md at line 31, The current reply_command uses the POST
/repos/{owner}/{repo}/pulls/{number}/comments endpoint which creates new review
comments; either rename reply_command to something like create_comment_command
if you intend to keep creating top-level comments, or change reply_command to
target the replies endpoint POST
/repos/{owner}/{repo}/pulls/{number}/comments/{comment_id}/replies and include a
{comment_id} placeholder in the command string and JSON payload so existing
comments are replied to (update the reply_command value accordingly and ensure
any code that writes the JSON temp file supplies comment_id).
There was a problem hiding this comment.
I’m going to keep the existing command here. This workflow schema intentionally uses reply_command with the pull comments endpoint plus an in_reply_to field in the JSON payload; that is what the heard-chef protocol documents, and it worked on this PR (the previous replies were created with in_reply_to_id set). Changing the endpoint would require changing the shared workflow format and runtime payload contract, which is outside this Phase 1 PR.
There was a problem hiding this comment.
@sandsower, understood — that's a valid approach. The POST /repos/{owner}/{repo}/pulls/{number}/comments endpoint does accept an in_reply_to field in the request body to thread a reply onto an existing comment, so the current reply_command is correct as-is. I'll note this for future reviews.
✏️ Learnings added
Learnt from: sandsower
Repo: sandsower/beislid PR: 74
File: .beislid/workflow.md:31-31
Timestamp: 2026-05-10T08:37:47.431Z
Learning: In `.beislid/workflow.md` (sandsower/beislid), the `pr_review_update` block's `reply_command` intentionally uses `POST /repos/{owner}/{repo}/pulls/{number}/comments` (not the `/replies` sub-endpoint). Replies to existing review comments are threaded by including an `in_reply_to` (or `in_reply_to_id`) field in the JSON payload written to `{json_file}`. This is per the "heard-chef" shared workflow protocol and has been verified to work in practice. Do not flag this endpoint as incorrect.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
1 participant
Summary
bin/beislidas a thin Bash command dispatcherscripts/install_lib.shbin_dirandcli_pathin the user install manifestghforheard-chefNotes
Project-level install remains deferred.
beislid install projectis not implemented in this slice.Verification
python3 scripts/validate_skills.pybash scripts/test_install.sh(35 passed, 0 failed)bash -n install.sh bin/beislid scripts/install_lib.sh scripts/test_install.shgit diff --checkpython3 tests/agent-smoke/run.py gate ship-it --hosts claude,codex --timeout 900 --changed-onlySummary by CodeRabbit
New Features
Documentation
Tests
Other