fix(agentic-engineering): expose the guarded classifier path - #205
Conversation
@coderabbitai full review Please review the current full head d0662db for correctness, security, and maintainability. Review the JSON path hint, retained denial, literal path quoting, relocated installation tests, and bounded surveyor recovery. Local verification: 393 guard cases, 35 adapter cases, 18 classifier cases, and 202 manifest cases passed; ShellCheck, desired-state digests, plugin version gate, and bundled-skill edit guard passed. Independent local correctness/security review found no actionable defects. User evaluation: the denied probe returned one trusted path; its remote-mode call passed the same guard and the real classifier returned green for agent-plugins main bb19a27. No installed runtime cache was modified. |
|
✅ Action performedFull review finished. |
|
Warning Review limit reachedNext included review available in 48 minutes. View limit detailsLimit 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. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Team Run ID: 📒 Files selected for processing (2)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Team Run ID: 📒 Files selected for processing (11)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📜 Recent review details⏰ Context from checks skipped due to timeout. (22)
🧰 Additional context used📓 Path-based instructions (2)Any change to the plugin set updates both manifests in the same PR Edit both together.📄 CodeRabbit inference engine (AGENTS.md) Files:
README and manifests stay in lockstep.📄 CodeRabbit inference engine (AGENTS.md) Files:
🪛 ast-grep (0.45.2)plugins/agentic-engineering/scripts/surveyor-forge-readonly.test.sh[error] 267-267: A variable, parameter expansion, or command-substitution result is passed as the command string to (bash-c-variable-injection-bash) 🔇 Additional comments (11)
📝 WalkthroughWalkthroughThe read-only guard now denies bare default-branch classifier probes while returning a JSON-encoded sibling path when available. The surveyor decodes this path as data and retries the command through the guard. Missing or invalid discovery data produces Merge Risk: ⚪ Minimal · up to The surveyor can now discover and invoke the installed classifier for remote default-branch checks while preserving guarded execution and fail-closed unknown results. The covered discovery and denial behavior leaves no merge-blocking risk. Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 3 functions across 4 files. (7 skipped: 7 unsupported.) 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 |
@coderabbitai review Please review current full head 966c942. The previous review's concrete docstring finding is fixed: classify_segment now documents its two arguments, admission/denial behavior, and denied discovery. The generated guard digest was refreshed; all 393 guard and 35 adapter cases and actual manifest validation pass again. Disposition of the ast-grep context warning in comment 5560626741: the test's bash -c exercises the exact command string accepted by the real guard. Before that call, the decoded path must equal the expected owned fixture path, jq @sh makes it one literal shell word, and the unchanged guard admits only that exact sibling and fixed remote-mode arguments. There is no unchecked dynamic command execution. The relocation case includes literal quotes and dollar signs and produces the expected named workflow result. No behavior changed in this documentation-only follow-up. |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
head='966c9422150d3df210053c9c9258b35c66c792d1'
git rev-parse --verify "${head}^{commit}"
base="$(git merge-base "${head}" origin/main 2>/dev/null || git merge-base "${head}" main)"
printf 'BASE=%s\nHEAD=%s\n' "$base" "$head"
git diff --stat "$base" "$head"
git diff --name-status "$base" "$head"
printf '\n--- changed diff ---\n'
git diff --find-renames --find-copies --unified=80 "$base" "$head" -- \
plugins/agentic-engineering/scripts/forge-readonly-guard.sh \
plugins/agentic-engineering/scripts/surveyor-forge-readonly.test.sh \
plugins/agentic-engineering/agents/portfolio-surveyor.agent.md \
plugins/agentic-engineering/README.md \
plugins/agentic-engineering/resources/provider-neutral.desired-state.json \
scripts/validate-manifests.sh \
scripts/validate-manifests.test.sh \
plugins/agentic-engineering/.claude-plugin/plugin.json \
plugins/agentic-engineering/plugin.json \
.claude-plugin/marketplace.json \
.github/plugin/marketplace.json
printf '\n--- guard outline ---\n'
ast-grep outline plugins/agentic-engineering/scripts/forge-readonly-guard.sh --items all || true
printf '\n--- relevant guard symbols and calls ---\n'
rg -n -C 12 -e '(^|[[:space:]])classify_segment[[:space:]]*\(' \
-e 'DEFAULT_BRANCH_CLASSIFIER' \
-e 'classifier-path-json:' \
-e 'classify-default-branch-ci-runs\.sh' \
plugins/agentic-engineering/scripts/forge-readonly-guard.sh
printf '\n--- relevant test construction and assertions ---\n'
rg -n -C 15 -e 'bash[[:space:]]+-c' \
-e 'jq[[:space:]].*`@sh`' \
-e 'classifier-path-json:' \
-e 'expected.*classifier|classifier.*expected' \
-e 'classify-default-branch-ci-runs\.sh' \
plugins/agentic-engineering/scripts/surveyor-forge-readonly.test.sh
printf '\n--- surveyor discovery instructions ---\n'
rg -n -C 12 -e 'classifier-path-json:' \
-e 'classify-default-branch-ci-runs\.sh' \
-e 'QUERY-UNKNOWN' \
-e 'literal' \
plugins/agentic-engineering/agents/portfolio-surveyor.agent.md \
plugins/agentic-engineering/README.mdLength of output: 174919 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '--- commits and follow-up diff ---'
git log --oneline --decorate -12 966c9422150d3df210053c9c9258b35c66c792d1
parent="$(git rev-parse 966c9422150d3df210053c9c9258b35c66c792d1^)"
printf 'PARENT=%s\n' "$parent"
git diff --check "$parent" 966c9422150d3df210053c9c9258b35c66c792d1
git diff --unified=30 "$parent" 966c9422150d3df210053c9c9258b35c66c792d1 -- \
plugins/agentic-engineering/scripts/forge-readonly-guard.sh \
plugins/agentic-engineering/resources/provider-neutral.desired-state.json
printf '%s\n' '--- guard parser and classifier admission ---'
rg -n -C 10 -e '^(tokenize_segment|scan_segments|classify_default_branch_ci|deny|die)[[:space:]]*\(' \
-e 'DEFAULT_BRANCH_CLASSIFIER' \
-e 'not the guarded remote-mode shape' \
plugins/agentic-engineering/scripts/forge-readonly-guard.sh
printf '%s\n' '--- adapter guard invocation and exit mapping ---'
ast-grep outline plugins/agentic-engineering/scripts/surveyor-forge-readonly.sh --items all || true
rg -n -C 12 -e 'SURVEYOR_FORGE_READONLY_GUARD' \
-e 'permissionDecisionReason' \
-e 'exit ' \
-e -- '--command' \
plugins/agentic-engineering/scripts/surveyor-forge-readonly.sh
printf '%s\n' '--- classifier argument parsing and remote API call ---'
ast-grep outline plugins/agentic-engineering/scripts/classify-default-branch-ci-runs.sh --items all || true
rg -n -C 12 -e '^(main|usage|die)[[:space:]]*\(' \
-e -- '--input' \
-e -- '--repo' \
-e -- '--branch' \
-e -- '--head-sha' \
-e 'gh api' \
plugins/agentic-engineering/scripts/classify-default-branch-ci-runs.sh
printf '%s\n' '--- desired-state digest verification ---'
expected_guard="$(jq -r '.spec.source.requiredRuntimeAssets[] | select(.path == "scripts/forge-readonly-guard.sh") | .sha256' plugins/agentic-engineering/resources/provider-neutral.desired-state.json)"
expected_agent="$(jq -r '.spec.roles["portfolio-surveyor"].definitionSha256' plugins/agentic-engineering/resources/provider-neutral.desired-state.json)"
actual_guard="$(sha256sum plugins/agentic-engineering/scripts/forge-readonly-guard.sh | awk '{print $1}')"
actual_agent="$(sha256sum plugins/agentic-engineering/agents/portfolio-surveyor.agent.md | awk '{print $1}')"
printf 'guard expected=%s actual=%s\nagent expected=%s actual=%s\n' \
"$expected_guard" "$actual_guard" "$expected_agent" "$actual_agent"
[ "$expected_guard" = "$actual_guard" ]
[ "$expected_agent" = "$actual_agent" ]
printf '%s\n' '--- manifest version consistency ---'
jq -r '.version' \
plugins/agentic-engineering/plugin.json \
plugins/agentic-engineering/.claude-plugin/plugin.json
jq -r '.plugins[] | select(.name == "agentic-engineering") | .version' \
.claude-plugin/marketplace.json .github/plugin/marketplace.jsonLength of output: 18279 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '--- command scanner and tokenizer ---'
sed -n '294,510p' plugins/agentic-engineering/scripts/forge-readonly-guard.sh
sed -n '508,690p' plugins/agentic-engineering/scripts/forge-readonly-guard.sh
printf '%s\n' '--- classifier admission ---'
sed -n '1385,1430p' plugins/agentic-engineering/scripts/forge-readonly-guard.sh
printf '%s\n' '--- adapter ---'
cat -n plugins/agentic-engineering/scripts/surveyor-forge-readonly.sh
printf '%s\n' '--- adapter guard call locations ---'
rg -n -F -C 10 -- '--command' plugins/agentic-engineering/scripts/surveyor-forge-readonly.sh
rg -n -F -C 10 -- 'permissionDecisionReason' plugins/agentic-engineering/scripts/surveyor-forge-readonly.sh
printf '%s\n' '--- classifier argument parsing and remote execution ---'
rg -n -F -C 12 -- '--repo' plugins/agentic-engineering/scripts/classify-default-branch-ci-runs.sh
rg -n -F -C 12 -- '--input' plugins/agentic-engineering/scripts/classify-default-branch-ci-runs.sh
rg -n -F -C 12 -- 'gh api' plugins/agentic-engineering/scripts/classify-default-branch-ci-runs.sh
printf '%s\n' '--- desired-state digest verification ---'
expected_guard="$(jq -r '.spec.source.requiredRuntimeAssets[] | select(.path == "scripts/forge-readonly-guard.sh") | .sha256' plugins/agentic-engineering/resources/provider-neutral.desired-state.json)"
expected_agent="$(jq -r '.spec.roles["portfolio-surveyor"].definitionSha256' plugins/agentic-engineering/resources/provider-neutral.desired-state.json)"
actual_guard="$(sha256sum plugins/agentic-engineering/scripts/forge-readonly-guard.sh | awk '{print $1}')"
actual_agent="$(sha256sum plugins/agentic-engineering/agents/portfolio-surveyor.agent.md | awk '{print $1}')"
printf 'guard expected=%s actual=%s\nagent expected=%s actual=%s\n' \
"$expected_guard" "$actual_guard" "$expected_agent" "$actual_agent"
test "$expected_guard" = "$actual_guard"
test "$expected_agent" = "$actual_agent"
printf '%s\n' '--- manifest version consistency ---'
jq -r '.version' \
plugins/agentic-engineering/plugin.json \
plugins/agentic-engineering/.claude-plugin/plugin.json
jq -r '.plugins[] | select(.name == "agentic-engineering") | .version' \
.claude-plugin/marketplace.json .github/plugin/marketplace.jsonLength of output: 29885
I found no actionable correctness, security, or maintainability issues.
The guard still admits only the exact absolute sibling path and the fixed remote-mode argument shape. Offline The relocation test validates the decoded path against the owned fixture path before it applies The desired-state digests match the guard and surveyor files. The plugin and marketplace versions are consistently ✏️ Learnings added
|
@codex review Please review current full head 966c942. CodeRabbit completed the previous head and its documentation finding is fixed, but its current-head request is explicitly rate limited (comment 5560662473), so that response does not satisfy current-head review. The change exposes the guard-owned classifier path as JSON in a denied discovery response, preserving the exact executable and remote-argument restrictions. Tests exercise discovery, adapter transport, quoting, relocation, actual classifier execution, and missing/unexecutable helpers. Local verification remains 393 guard, 35 adapter, 18 classifier, and 202 manifest cases, with ShellCheck, digests, version parity, and a live guarded CI read passing. |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Codex Review: Didn't find any major issues. Another round soon, please! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
1 participant
The portfolio surveyor could not locate the only CI classifier its read-only guard permits, leaving default-branch health unknown even when the helper was installed. The guard now returns its own classifier path as JSON in a denied discovery response; the surveyor can use that literal path for the existing guarded call.
Plugin 5.0.1 documents the bounded discovery flow and preserves denial of arbitrary executables, offline input, and malformed calls. A relocated-installation test exercises discovery through the adapter, admission through the guard, and the real classifier's named workflow result.
Fixes #200