fix: post-1.0.0 code review — ponytail custom skills, auth health scoring, CI defects - #94
Merged
Merged
Conversation
…ng to global - custom skill names now dispatch: normalize_extended_mode accepts runtime-discovered names in switcher, session, and default paths - build() delivers the custom skill body inline instead of collapsing to the default mode and pointing at a nonexistent harness skill - reserved built-in names (full.md etc.) can no longer shadow modes - SessionStart clears the session-scoped override (no SessionEnd hook exists, so a stale override was promoted into the global flag) - skill-help.md documented the wrong config path and JSON key
record_error had no production callers and last_used_at was never written, so smart_pick scored every profile identically. The runner now records errors on rate-limit and failure exits; activation stamps last_used_at via the new touch_last_used.
… env lock - lookup_pricing: longest matching prefix wins instead of random HashMap iteration order picking an arbitrary rate - rollup sync prunes catalog/rollup/dedup rows for deleted session files (stale rows overstated cost forever and could suppress lines of new files as false duplicates) - get_routing_suggestion uses active_router() like the CLI hook - paths.rs test env mutation now shares agent-registry's PATH_LOCK: two independent locks let set_var calls race across threads - agent-registry: mechanical clippy fixes
- winget: asset filter matched only .exe/.msi but releases ship a .zip, so every release run threw 'No winget installer assets found' - security-check: drop paths filter — cargo audit is a required check and never reported on PRs not touching manifests, deadlocking merges - ppa-publish: upload-artifact pinned to a malformed 42-char SHA - docker: 'latest' tag was gated on a condition that is always false under the workflow's only trigger - vendored-file-warning: exempt bot authors (Renovate)
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThis PR updates workflow and release tooling, hardens install/container behavior, and changes ponytail, auth, routing, pricing, and rollup logic. ChangesCI, container, and release tooling
Runtime behavior changes
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant AuthCLI as auth CLI
participant ActivateWith as activate_with
participant AuthDB as auth_db
participant AuthRunner as auth_runner
AuthCLI->>ActivateWith: activate profile
ActivateWith->>AuthDB: touch_last_used(agent, profile)
ActivateWith-->>AuthCLI: activation result
AuthRunner->>AuthRunner: spawn_and_capture
AuthRunner->>AuthRunner: categorize_exit(code, stderr)
alt RateLimited
AuthRunner->>AuthDB: record_error(agent, profile, stderr)
else Failure
AuthRunner->>AuthDB: record_error(agent, profile, stderr)
end
sequenceDiagram
participant User as user
participant Switcher as switcher::detect
participant Config as config::normalize_extended_mode
participant SubSkills as sub_skills
participant Instructions as instructions::build
User->>Switcher: /ponytail:{name}
Switcher->>Config: normalize_extended_mode(name)
Config->>SubSkills: get_custom(name) when needed
Config-->>Switcher: normalized mode
Switcher->>Instructions: build(mode, skill_path)
Instructions->>SubSkills: get(effective)
alt installed skill
SubSkills-->>Instructions: skill body
else custom skill
Instructions->>SubSkills: get_custom(effective)
SubSkills-->>Instructions: custom body
end
Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
onpush_run.log, install_onpush.ps1, and run_onpush.bat are machine-local doc-generation automation (run_onpush.bat hard-codes a local path); they were swept into 853e4c3 by accident. Removed from tracking and gitignored along with the rest of the onpush artifacts.
zizmor's impostor-commit audit flagged the old pin: the SHA resolves via the commits API (fork network) but is not in dtolnay/rust-toolchain's actual branch history. Repinned to the current stable branch head.
- Dockerfile: install.sh was fetched from a nonexistent 'main' branch so the image could never build; also drop unused sudo and run as a non-root user - install.sh: fail closed when SHA256SUMS is missing or lacks the asset line (AGENTFLARE_SKIP_VERIFY=1 to bypass); never source-build from an incidental pwd when piped via curl|sh - release-npm.sh: always re-download release assets (stale unversioned files in a reused RELEASE_DIR republished old binaries); exit non-zero when any declared platform asset is missing instead of publishing an incomplete set
Owner
Author
|
Follow-up commits after the gap-closure review of scripts/Docker/build.rs (files the first pass didn't cover):
Not fixed (tracked): Docker image builds aren't reproducible (installs latest from master at build time — needs a version build-arg through install.sh); |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
crates/ponytail/src/switcher.rs (1)
100-149: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding a test for custom-skill-name dispatch.
Existing tests (
detects_mode_switch,detects_default,detects_session_mode) only cover the fixedlite/full/ultramodes. Since this PR's core fix is custom-mode dispatch vianormalize_extended_mode, a test exercisingSwitchAction::SetMode/SetDefault/SetSessionwith a custom skill name (via a mockedget_custom) would directly validate the fix.🤖 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 `@crates/ponytail/src/switcher.rs` around lines 100 - 149, Add a test in switcher.rs that exercises custom-skill-name dispatch through detect and normalize_extended_mode by mocking get_custom to return a custom skill name; verify SwitchAction::SetMode, SwitchAction::SetDefault, and SwitchAction::SetSession resolve correctly for that custom name instead of only the built-in lite/full/ultra cases. Place it alongside the existing detects_mode_switch, detects_default, and detects_session_mode tests so the new coverage directly validates the PR’s custom mode fix.src/auth_runner.rs (1)
15-53: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueHealth-scoring wiring looks correct; consider deduplicating the repeated recording pattern.
The
(code, stderr)capture andcategorize_exit(code, &stderr)call are correct, and bothRateLimited(Lines 30-34) andFailure(Lines 42-48) branches correctly reuse the same connection for their respective operations with proper parameter passing torecord_error.The
let conn = auth_db::open_or_rebuild(); if let Some((profile, _)) = auth_db::get_rotation_last(&conn, agent) { auth_db::record_error(&conn, agent, &profile, &stderr); ... }pattern is duplicated across both branches. Extracting a small helper (e.g.,record_last_profile_error(agent, stderr) -> Option<Connection>) would reduce duplication if this logic grows further.🤖 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 `@src/auth_runner.rs` around lines 15 - 53, The repeated auth_db recording logic in auth_runner::run should be deduplicated. Extract the shared pattern that opens the connection, looks up the last rotated profile with auth_db::get_rotation_last, and records the stderr via auth_db::record_error into a small helper (for example, a helper tied to run or auth_db). Then update both the ExitKind::RateLimited and ExitKind::Failure branches to call that helper, keeping the existing behavior unchanged while removing the duplicated block.
🤖 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 `@crates/ponytail/src/config.rs`:
- Around line 24-31: Update the mode parsing used by PonytailAction::Set and
PonytailAction::Default so it routes through normalize_extended_mode instead of
normalize_config_mode. The current path in the cli/ponytail handling rejects
runtime-discovered custom skill names as invalid before they can reach
set_default_mode or set_active, so make the action-specific parsing consistent
with PromptSubmit by accepting both built-in and custom modes via the extended
helper.
In `@crates/ponytail/src/sub_skills.rs`:
- Around line 39-43: The reserved-name guard in sub_skills.rs currently compares
path.file_stem() directly against VALID_MODES, so mixed-case names can bypass
the check and be added to CUSTOM_SKILLS. Update the name handling in the
custom-skill discovery path to normalize the stem before the
VALID_MODES.contains(...) check, using the existing logic around
path.file_stem() and the CUSTOM_SKILLS insertion flow, so reserved built-in
modes remain blocked case-insensitively.
In `@src/auth.rs`:
- Around line 210-212: The success path in auth handling is blocked by the DB
health update, so move the success output to happen before the call into
auth_db::touch_last_used, or make that touch best-effort after printing success.
Use the open_or_rebuild() flow in src/auth.rs and the touch_last_used helper as
the key points to reorder so a transient auth.db failure cannot prevent the
success message from being emitted.
In `@src/rollup.rs`:
- Around line 219-228: The per-statement `.ok()` calls inside the stale-path
cleanup are swallowing errors and breaking the atomicity of the transaction.
Update the cleanup logic in the function that iterates over `stale` to follow
the same fail-fast pattern used by `reindex_file`: if any `tx.execute(...)` call
fails, abort the function immediately so the transaction rolls back on drop,
instead of continuing and committing partial deletes. Keep the
`conn.transaction()` / `tx.commit()` flow, but ensure a failure in any of the
`DELETE` statements prevents commit.
---
Nitpick comments:
In `@crates/ponytail/src/switcher.rs`:
- Around line 100-149: Add a test in switcher.rs that exercises
custom-skill-name dispatch through detect and normalize_extended_mode by mocking
get_custom to return a custom skill name; verify SwitchAction::SetMode,
SwitchAction::SetDefault, and SwitchAction::SetSession resolve correctly for
that custom name instead of only the built-in lite/full/ultra cases. Place it
alongside the existing detects_mode_switch, detects_default, and
detects_session_mode tests so the new coverage directly validates the PR’s
custom mode fix.
In `@src/auth_runner.rs`:
- Around line 15-53: The repeated auth_db recording logic in auth_runner::run
should be deduplicated. Extract the shared pattern that opens the connection,
looks up the last rotated profile with auth_db::get_rotation_last, and records
the stderr via auth_db::record_error into a small helper (for example, a helper
tied to run or auth_db). Then update both the ExitKind::RateLimited and
ExitKind::Failure branches to call that helper, keeping the existing behavior
unchanged while removing the duplicated block.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: a53fc6a8-594c-436e-98c3-7472fd3f52f7
📒 Files selected for processing (20)
.github/workflows/docker.yml.github/workflows/ppa-publish.yml.github/workflows/security-check.yml.github/workflows/vendored-file-warning.yml.github/workflows/winget.ymlcrates/agent-registry/src/detect.rscrates/agent-registry/src/registry.rscrates/ponytail/src/config.rscrates/ponytail/src/instructions.rscrates/ponytail/src/skill-help.mdcrates/ponytail/src/sub_skills.rscrates/ponytail/src/switcher.rssrc/auth.rssrc/auth_db.rssrc/auth_runner.rssrc/cli/ponytail.rssrc/mcp_server.rssrc/paths.rssrc/pricing.rssrc/rollup.rs
Comment on lines
+24
to
+31
| /// Like `normalize_config_mode`, but also accepts user-defined custom skill | ||
| /// names (which are discovered at runtime, so they can't be `&'static str`). | ||
| pub fn normalize_extended_mode(mode: &str) -> Option<String> { | ||
| let m = mode.trim().to_lowercase(); | ||
| normalize_config_mode(&m) | ||
| .map(str::to_string) | ||
| .or_else(|| crate::sub_skills::get_custom(&m).map(|_| m)) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== config.rs matches ==\n'
rg -n "normalize_extended_mode|normalize_config_mode|get_custom|set_default_mode|set_active" crates/ponytail/src/config.rs src/cli/ponytail.rs || true
printf '\n== config.rs excerpt ==\n'
sed -n '1,120p' crates/ponytail/src/config.rs
printf '\n== cli excerpt ==\n'
sed -n '1,240p' src/cli/ponytail.rsRepository: getappz/agentflare
Length of output: 13666
Wire PonytailAction::Set/Default through normalize_extended_mode — src/cli/ponytail.rs:104-125 still uses normalize_config_mode, so direct set/default commands reject custom skill names as invalid mode before they reach set_default_mode/set_active. PromptSubmit already uses the extended path.
🤖 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 `@crates/ponytail/src/config.rs` around lines 24 - 31, Update the mode parsing
used by PonytailAction::Set and PonytailAction::Default so it routes through
normalize_extended_mode instead of normalize_config_mode. The current path in
the cli/ponytail handling rejects runtime-discovered custom skill names as
invalid before they can reach set_default_mode or set_active, so make the
action-specific parsing consistent with PromptSubmit by accepting both built-in
and custom modes via the extended helper.
Comment on lines
+39
to
+43
| // Reserved built-in mode/skill names can't be shadowed by | ||
| // custom files — a user's full.md must not hijack full mode. | ||
| if crate::config::VALID_MODES.contains(&name) { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm VALID_MODES contents and whether reserved skill names overlap.
rg -n 'VALID_MODES' crates/ponytail/src/config.rs -A5 -B2
rg -n 'fn normalize_mode|fn all_mode_names|fn all_skill_names' crates/ponytail/src/config.rs crates/ponytail/src/switcher.rs -A10Repository: getappz/agentflare
Length of output: 2825
🏁 Script executed:
#!/bin/bash
sed -n '1,220p' crates/ponytail/src/sub_skills.rs
sed -n '1,120p' crates/ponytail/src/config.rs
sed -n '1,120p' crates/ponytail/src/switcher.rsRepository: getappz/agentflare
Length of output: 13335
Normalize the reserved-name check crates/ponytail/src/sub_skills.rs:39-43
path.file_stem() is compared raw against VALID_MODES, so Full.md/FULL.md slips past the guard and gets added to CUSTOM_SKILLS. Normalize the stem before checking so reserved names stay blocked case-insensitively.
🤖 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 `@crates/ponytail/src/sub_skills.rs` around lines 39 - 43, The reserved-name
guard in sub_skills.rs currently compares path.file_stem() directly against
VALID_MODES, so mixed-case names can bypass the check and be added to
CUSTOM_SKILLS. Update the name handling in the custom-skill discovery path to
normalize the stem before the VALID_MODES.contains(...) check, using the
existing logic around path.file_stem() and the CUSTOM_SKILLS insertion flow, so
reserved built-in modes remain blocked case-insensitively.
Comment on lines
+210
to
+212
| let conn = crate::auth_db::open_or_rebuild(); | ||
| crate::auth_db::touch_last_used(&conn, agent, &profile); | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '--- src/auth.rs outline ---\n'
ast-grep outline src/auth.rs --view expanded || true
printf '\n--- src/auth_db.rs outline ---\n'
ast-grep outline src/auth_db.rs --view expanded || true
printf '\n--- relevant references to open_or_rebuild/touch_last_used/activate ---\n'
rg -n "open_or_rebuild|touch_last_used|activate|restored|success" src/auth.rs src/auth_db.rs src/auth_runner.rs
printf '\n--- src/auth.rs around the reported lines ---\n'
sed -n '180,240p' src/auth.rs
printf '\n--- src/auth_db.rs around open_or_rebuild/touch_last_used ---\n'
sed -n '1,220p' src/auth_db.rs
printf '\n--- src/auth_runner.rs relevant branch ---\n'
sed -n '1,220p' src/auth_runner.rsRepository: getappz/agentflare
Length of output: 247
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- src/auth.rs outline ---'
ast-grep outline src/auth.rs --view expanded || true
printf '\n%s\n' '--- src/auth_db.rs outline ---'
ast-grep outline src/auth_db.rs --view expanded || true
printf '\n%s\n' '--- relevant references to open_or_rebuild/touch_last_used/activate ---'
rg -n "open_or_rebuild|touch_last_used|activate|restored|success" src/auth.rs src/auth_db.rs src/auth_runner.rs
printf '\n%s\n' '--- src/auth.rs around the reported lines ---'
sed -n '180,240p' src/auth.rs
printf '\n%s\n' '--- src/auth_db.rs around open_or_rebuild/touch_last_used ---'
sed -n '1,220p' src/auth_db.rs
printf '\n%s\n' '--- src/auth_runner.rs relevant branch ---'
sed -n '1,220p' src/auth_runner.rsRepository: getappz/agentflare
Length of output: 24883
Move the success output before the DB touch. src/auth.rs:210-220 — open_or_rebuild() can still panic on open/migration failures, so a transient auth.db issue can turn a successful restore into a crash before the success message is printed. Make this health update best-effort, or at least emit the success output before calling into the DB.
🤖 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 `@src/auth.rs` around lines 210 - 212, The success path in auth handling is
blocked by the DB health update, so move the success output to happen before the
call into auth_db::touch_last_used, or make that touch best-effort after
printing success. Use the open_or_rebuild() flow in src/auth.rs and the
touch_last_used helper as the key points to reorder so a transient auth.db
failure cannot prevent the success message from being emitted.
Comment on lines
+219
to
+228
| let Ok(tx) = conn.transaction() else { | ||
| return; | ||
| }; | ||
| for path in stale { | ||
| tx.execute("DELETE FROM session_files WHERE file_path = ?1", params![path]).ok(); | ||
| tx.execute("DELETE FROM file_rollup WHERE file_path = ?1", params![path]).ok(); | ||
| tx.execute("DELETE FROM dedup_keys WHERE file_path = ?1", params![path]).ok(); | ||
| } | ||
| tx.commit().ok(); | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Per-statement .ok() breaks the transaction's atomicity guarantee.
Each DELETE inside the loop swallows its own error and the loop continues, so a mid-loop failure (lock contention, I/O error) can leave a stale path partially deleted (e.g. session_files row gone but file_rollup/dedup_keys rows still present) and tx.commit() will still succeed. Since the path is then gone from session_files, the next sync() run's known list won't include it either, so it will never be pruned again — permanently reintroducing the exact "stale rows overstate cost forever" bug the doc comment (lines 198-200) says this function prevents.
reindex_file in this same file already uses the correct pattern (if tx.execute(...).is_err() { return; }, letting the transaction roll back on drop) — this function should follow the same convention.
🔧 Proposed fix to abort on per-statement failure
let Ok(tx) = conn.transaction() else {
return;
};
for path in stale {
- tx.execute("DELETE FROM session_files WHERE file_path = ?1", params![path]).ok();
- tx.execute("DELETE FROM file_rollup WHERE file_path = ?1", params![path]).ok();
- tx.execute("DELETE FROM dedup_keys WHERE file_path = ?1", params![path]).ok();
+ if tx.execute("DELETE FROM session_files WHERE file_path = ?1", params![path]).is_err() {
+ return;
+ }
+ if tx.execute("DELETE FROM file_rollup WHERE file_path = ?1", params![path]).is_err() {
+ return;
+ }
+ if tx.execute("DELETE FROM dedup_keys WHERE file_path = ?1", params![path]).is_err() {
+ return;
+ }
}
tx.commit().ok();
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| let Ok(tx) = conn.transaction() else { | |
| return; | |
| }; | |
| for path in stale { | |
| tx.execute("DELETE FROM session_files WHERE file_path = ?1", params![path]).ok(); | |
| tx.execute("DELETE FROM file_rollup WHERE file_path = ?1", params![path]).ok(); | |
| tx.execute("DELETE FROM dedup_keys WHERE file_path = ?1", params![path]).ok(); | |
| } | |
| tx.commit().ok(); | |
| } | |
| let Ok(tx) = conn.transaction() else { | |
| return; | |
| }; | |
| for path in stale { | |
| if tx.execute("DELETE FROM session_files WHERE file_path = ?1", params![path]).is_err() { | |
| return; | |
| } | |
| if tx.execute("DELETE FROM file_rollup WHERE file_path = ?1", params![path]).is_err() { | |
| return; | |
| } | |
| if tx.execute("DELETE FROM dedup_keys WHERE file_path = ?1", params![path]).is_err() { | |
| return; | |
| } | |
| } | |
| tx.commit().ok(); | |
| } |
🤖 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 `@src/rollup.rs` around lines 219 - 228, The per-statement `.ok()` calls inside
the stale-path cleanup are swallowing errors and breaking the atomicity of the
transaction. Update the cleanup logic in the function that iterates over `stale`
to follow the same fail-fast pattern used by `reindex_file`: if any
`tx.execute(...)` call fails, abort the function immediately so the transaction
rolls back on drop, instead of continuing and committing partial deletes. Keep
the `conn.transaction()` / `tx.commit()` flow, but ensure a failure in any of
the `DELETE` statements prevents commit.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
Dockerfile (1)
5-14: 🧹 Nitpick | 🔵 TrivialNon-root user setup looks correct.
apt-get steps still run as root before the
USERswitch, andPATH/WORKDIRare consistently updated for the new user.Reminder: image builds remain non-reproducible.
Line 12 still installs from
masterat build time, so image builds aren't pinned to a specific release, which the PR itself flags as a known gap requiring a version build-arg threaded throughinstall.sh.🤖 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 `@Dockerfile` around lines 5 - 14, The non-root setup is fine, but the install step in the Dockerfile still pulls install.sh from the moving master branch, making builds non-reproducible. Update the Dockerfile’s curl | sh install step to use a pinned release/version source instead of master, and thread that version through install.sh using the existing version build-arg approach so the install is deterministic.
🤖 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 `@install.sh`:
- Around line 138-157: The fail-closed verification flow still allows unverified
installs when neither checksum tool is present because verify_checksum()
succeeds in that case. Update verify_checksum() so the missing-tool path is
treated like any other verification failure unless AGENTFLARE_SKIP_VERIFY=1 is
explicitly set, and make sure the install.sh checksum handling around
verify_checksum and the SHA256SUMS download branch consistently enforces the
same opt-in bypass.
---
Nitpick comments:
In `@Dockerfile`:
- Around line 5-14: The non-root setup is fine, but the install step in the
Dockerfile still pulls install.sh from the moving master branch, making builds
non-reproducible. Update the Dockerfile’s curl | sh install step to use a pinned
release/version source instead of master, and thread that version through
install.sh using the existing version build-arg approach so the install is
deterministic.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 2d5d8f83-d0ca-43ff-8acb-45d92df98e13
⛔ Files ignored due to path filters (1)
onpush_run.logis excluded by!**/*.log
📒 Files selected for processing (7)
.github/workflows/ppa-publish.yml.gitignoreDockerfileinstall.shinstall_onpush.ps1run_onpush.batscripts/release-npm.sh
💤 Files with no reviewable changes (2)
- run_onpush.bat
- install_onpush.ps1
✅ Files skipped from review due to trivial changes (1)
- .gitignore
Comment on lines
+138
to
+157
| # Fail closed: an installer that silently skips verification when the | ||
| # (much smaller) SHA256SUMS request is blocked or incomplete is exactly | ||
| # what a selective MITM wants. AGENTFLARE_SKIP_VERIFY=1 is the escape hatch. | ||
| if curl -fsSL "$sums_url" -o "$tmpdir/SHA256SUMS" 2>/dev/null; then | ||
| expected="$(grep "agentflare-${target}.tar.gz" "$tmpdir/SHA256SUMS" | cut -d' ' -f1)" | ||
| if [ -n "$expected" ]; then | ||
| verify_checksum "$tmpdir/agentflare.tar.gz" "$expected" | ||
| elif [ "${AGENTFLARE_SKIP_VERIFY:-0}" = "1" ]; then | ||
| echo " Warning: agentflare-${target}.tar.gz not listed in SHA256SUMS — proceeding unverified (AGENTFLARE_SKIP_VERIFY=1)" | ||
| else | ||
| echo "Error: agentflare-${target}.tar.gz is not listed in SHA256SUMS — refusing to install unverified." | ||
| echo "Set AGENTFLARE_SKIP_VERIFY=1 to bypass (not recommended)." | ||
| exit 1 | ||
| fi | ||
| elif [ "${AGENTFLARE_SKIP_VERIFY:-0}" = "1" ]; then | ||
| echo " Warning: checksums not available — proceeding unverified (AGENTFLARE_SKIP_VERIFY=1)" | ||
| else | ||
| echo " Warning: checksums not available, skipping verification" | ||
| echo "Error: could not download SHA256SUMS — refusing to install unverified." | ||
| echo "Set AGENTFLARE_SKIP_VERIFY=1 to bypass (not recommended)." | ||
| exit 1 |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the relevant file and inspect the installer around the cited lines.
git ls-files install.sh
wc -l install.sh
sed -n '80,170p' install.sh | cat -nRepository: getappz/agentflare
Length of output: 4055
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search for checksum helpers and any override usage.
rg -n "verify_checksum|AGENTFLARE_SKIP_VERIFY|sha256sum|shasum" install.shRepository: getappz/agentflare
Length of output: 1119
Gate the missing-hash-tool path behind the same opt-in
verify_checksum() still returns success when neither sha256sum nor shasum is installed, so a machine without either tool will proceed unverified unless AGENTFLARE_SKIP_VERIFY=1 is set. That leaves a bypass in the default fail-closed flow.
🤖 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 `@install.sh` around lines 138 - 157, The fail-closed verification flow still
allows unverified installs when neither checksum tool is present because
verify_checksum() succeeds in that case. Update verify_checksum() so the
missing-tool path is treated like any other verification failure unless
AGENTFLARE_SKIP_VERIFY=1 is explicitly set, and make sure the install.sh
checksum handling around verify_checksum and the SHA256SUMS download branch
consistently enforces the same opt-in bypass.
…-9, 11-14) Workflow-level contents: read on release.yml, ppa-publish.yml, and winget.yml, with per-job elevation only where the token is actually used for writes: the release job (creates the release + uploads assets) gets contents: write, the three dispatch jobs get actions: write. Homebrew/Scoop jobs push with dedicated PATs, not GITHUB_TOKEN.
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
1 participant
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.
What
High-effort code review of all commits after v1.0.0 (149 commits, 127 files); 12 verified findings fixed across four logical commits.
Critical
normalize_config_modeonly knows built-ins), collapsed to the default mode inbuild(), and the authored body was never delivered. Now wired end-to-end vianormalize_extended_mode+ inline body delivery; reserved names can't be shadowed by custom files..exe|.msibut releases ship a.zip.Major
record_errorhad zero production callers,last_used_atwas never written →smart_pickscored every profile identically. Runner now records errors on rate-limit/failure exits; activation stampslast_used_at.latesttag gated on a condition that's always false under the workflow's only trigger.paths.rsandagents.rsserializedset_varunder two different locks in one test binary; unified on agent-registry'sPATH_LOCK.Minor
syncprunes rows for deleted session files (costs no longer overstated forever; stale dedup ownership released).get_routing_suggestionhonorsAGENTFLARE_ROUTERviaactive_router()like the CLI hook.Deliberately not fixed (tracked)
filter_skill_bodyprose false-positives (marker syntax is a product decision) · AUR pinned at 1.0.1 with no release automation · stagederrors.rsenums / unusedRouteContextfields (intentional per commit history).Testing
cargo test --workspace: 269 passed / 0 failed.cargo build: zero errors, no new warnings. New behavior covered by existing suites; ponytail 33/33.Summary by CodeRabbit