Restore retained command dispatch and review correctness improvements#30
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
dceoy
left a comment
There was a problem hiding this comment.
The direction is substantially better than the reverted architecture, but the current implementation still leaves the original mutation boundary unenforced and has correctness gaps in agent selection and stale-head handling. Please address the inline findings before merge.
|
OpenCode PR Review: Submission Failed The token gate refused to submit: 1 candidate App token found in git credential configuration, but none verified as Inline findings (6)
Summary-only finding (1)
|
dceoy
left a comment
There was a problem hiding this comment.
The stale-head fixes are sound and CI is green, but the review-only boundary is still bypassable through project command/agent configuration and unrestricted subagent dispatch. I also found two input/config handling regressions. These should be fixed before merge.
dceoy
left a comment
There was a problem hiding this comment.
PR Review Summary
No new actionable issues meeting the review’s reporting threshold were identified beyond findings already present in unresolved review threads.
Checked
- Current head SHA and eligibility metadata
- Full PR diff and changed-file scope
- Slash-command expansion and agent-selection behavior
- Review-only trust boundary and project configuration precedence
- Stale-head guards and structured review submission path
- Existing review threads and duplicate-finding suppression
- Current-head CI/CD status
Notes
Existing unresolved findings remain applicable and were not duplicated, including project command/agent shadowing of the review boundary, unrestricted subagent dispatch, JSONC handling in OPENCODE_CONFIG_CONTENT, and literal-unsafe $ARGUMENTS substitution. Current-head CI/CD completed successfully.
Recommended Action
Address the existing unresolved critical and important review threads before merge.
dceoy
left a comment
There was a problem hiding this comment.
The latest fixes address the previously reported command-precedence, config-order, and stale-head gaps, and CI is green. Four blockers remain: the orchestrator's bash policy is still write-capable, project agent prompts remain shadowable, quoted YAML agent values are misparsed, and the 422 retry path can submit after the head moves.
dceoy
left a comment
There was a problem hiding this comment.
PR Review Summary
No additional actionable issues meeting the review’s reporting threshold were identified beyond the existing unresolved review threads.
Checked
- Current eligibility metadata, base/head refs, and exact head SHA.
- Complete changed-file patch for slash-command expansion, agent/config overlays, stale-head guards, and review-only permission enforcement.
- Existing review threads, including the latest unresolved trust-boundary, YAML parsing, and retry freshness findings.
- Current-head CI/CD status.
Notes
- Four existing unresolved current-head findings remain applicable and were not duplicated: generic write-capable
bash/gh apiaccess in the orchestrator, incomplete pinning of bundled agent definitions, quoted YAMLagentscalar parsing, and the missing freshness guard before the 422 retry/fallback POST. - Current-head CI/CD completed successfully.
Recommended Action
Address the four unresolved review threads before merge.
dceoy
left a comment
There was a problem hiding this comment.
PR Review Summary
Important Issues
-
[HIGH][Confidence: HIGH][security and trust boundaries] The trusted review helpers are loaded from a non-authoritative global-config path —
.opencode/scripts/review-pr-context.sh:25- Impact:
Copy bundled OpenCode configusescp -rn, so an existing${HOME}/.config/opencode/scripts/review-pr-context.sh,review-pr-submit.sh, orresolve-app-token.shis preserved. The new orchestrator then invokes and sources those preserved files as trusted code. A pre-populated runner or earlier workflow step can therefore replace the context/state derivation or GitHub submission helper, bypassing the exact-command allowlist, pinned-head checks, target binding, and review-only boundary. - Recommendation: execute/source the helpers directly from the immutable action checkout (
${{ github.action_path }}), or copy them into a dedicated per-run directory with overwrite plus ownership/permission validation. Do not treat~/.config/opencode/scriptsas trusted whencp -npermits pre-existing files to win.
- Impact:
Checked
- Current-head changes for command expansion, full bundled-agent overlay pinning, project-config disabling, the exact orchestrator permission allowlist, trusted context/submission helpers, stale-head gates, and related Bats validation.
- Existing review threads and their fixes on the current head.
- Current-head CI/CD result; the workflow completed successfully.
Strengths
- The current revision removes generic shell access from the orchestrator and binds structured review submissions to pinned per-run state.
- Previously reported command-shadowing, agent-shadowing, quoted-scalar, and retry-freshness defects are addressed.
Recommended Action
Fix the helper provenance issue before merge; the new security boundary otherwise depends on mutable pre-existing files outside the action checkout.
dceoy
left a comment
There was a problem hiding this comment.
The latest head resolves the previous review threads and CI passes, but three remaining issues still weaken the review-only boundary or introduce destructive runner behavior. Please address the inline findings before merge.
dceoy
left a comment
There was a problem hiding this comment.
The prior findings are addressed and CI is green, but the current head still has two security-boundary breaks and two correctness gaps. In particular, the supposedly trusted review state remains writable/pre-seedable under realistic workflow conditions, and the read-only agents can read checkout-persisted credentials. Please address the inline findings before merge.
dceoy
left a comment
There was a problem hiding this comment.
PR Review Summary
No new actionable issues meeting the review’s reporting threshold were identified beyond findings already present in unresolved review threads.
Checked
- Current eligibility metadata, base/head refs, and exact current head SHA.
- Full changed-file scope and current patches for slash-command expansion, bundled-agent pinning, project-config disabling, trusted context/submission helpers, state/payload directory handling, target binding, and stale-head gates.
- Existing review submissions and unresolved inline threads for duplicate suppression.
- Current-head GitHub Actions status; CI/CD completed successfully.
Notes
Four existing unresolved current-head findings remain applicable and were not duplicated: caller-controlled/pre-seedable review state, review-agent access to checkout-persisted .git/config credentials, context reads not explicitly bound to GITHUB_REPOSITORY, and the missing pinned-head gate before review-body updates.
Recommended Action
Address the four unresolved security and correctness threads before merge.
1325f95 to
15d553c
Compare
ed04240 to
546952b
Compare
516ff77 to
6ce6617
Compare
dceoy
left a comment
There was a problem hiding this comment.
Found two integration gaps in the retained functionality: comment-triggered slash commands are not expanded, and the structured review helper does not actually enforce a strict per-comment schema.
dceoy
left a comment
There was a problem hiding this comment.
PR Review Summary
No new actionable issues meeting the review’s reporting threshold were identified beyond findings already present in unresolved review threads.
Checked
- Repository ownership and archived status, PR author/state/draft status, base/head refs, and exact current head SHA.
- Full changed-file patch for command expansion, agent/config overlay handling, review context gathering, payload validation, stale-head protection, and review submission.
- Existing review submissions and unresolved inline threads for duplicate suppression.
- Current-head CI/CD status; the workflow completed successfully.
Notes
Two unresolved current-head findings remain applicable and were not duplicated: comment-triggered slash commands are not expanded before opencode github run, and the structured review helper does not enforce exact mutually consistent schemas for file-level versus line-level comments.
Recommended Action
Address the two unresolved important review threads before merge.
6ce6617 to
68c39ac
Compare
dceoy
left a comment
There was a problem hiding this comment.
The reduced one-commit shape is substantially better and CI is green, but three correctness gaps remain in the retained generic command/review functionality. In particular, the comment-triggered path is not portable to Bash 3.2, valid OpenCode commands can silently execute with different agent/model semantics, and the accepted file-level review payload is incompatible with the batch review endpoint. I would fix these before merge without broadening the architecture.
61838e9 to
459d20e
Compare
dceoy
left a comment
There was a problem hiding this comment.
The previous findings are fixed, but three user-visible regressions remain in the command-dispatch/install boundary. Please address the inline findings before merge; each can be fixed without broadening the PR architecture.
|
I edited |
dceoy
left a comment
There was a problem hiding this comment.
PR Review Summary
No new actionable issues meeting the review’s reporting threshold were identified for this head after deduplicating against existing review threads.
Checked
- Current diff across command expansion, action wiring, structured review submission, bundled review command/agent configuration, tests, and README contract.
- Comment-triggered prompt extraction and fallback behavior.
- Unsupported Markdown command semantics and literal argument expansion.
- Structured inline-comment payload validation, target binding, and stale-head checks.
- Current-head CI/CD status for
459d20ea0eb26af2c0ebf28988e40b1d8d7b2168.
Notes
Three existing unresolved findings remain applicable and were not duplicated: ordinary comment text can be truncated when event-derived text replaces the native prompt; unsupported positional placeholders or shell template blocks are not rejected; and toolkit installation force-overwrites user-global helper scripts despite the preservation contract.
Recommended Action
Address the existing unresolved findings before merge. No additional automated blocking concern was identified beyond those threads; merge remains a maintainer decision after required checks pass.
dceoy
left a comment
There was a problem hiding this comment.
PR Review Summary
No actionable issues meeting the review’s reporting threshold were identified.
Checked
- Repository ownership, archived status, PR author/state/draft status, base/head refs, and exact current head SHA.
- Full changed-file scope and current patches for Markdown slash-command resolution, event-comment extraction, frontmatter validation, literal argument expansion, JSONC config merging, and action wiring.
- Structured review payload schema validation, target derivation, token identity gate, and stale-head protection.
- Existing review submissions and resolved/unresolved inline threads for duplicate suppression.
- Current-head CI/CD status; the workflow completed successfully.
Notes
The current revision intentionally removes the earlier review-only orchestrator architecture and retains the narrower generic command-dispatch and structured-review helper functionality. The combined-status endpoint returned no status contexts; the current-head GitHub Actions CI/CD run completed successfully.
Recommended Action
No automated blocking concerns were identified. Merge remains a maintainer decision after required checks pass.
Summary
Recovers the independently useful functionality from reverted PRs #27 and #28 without reintroducing their #24-specific review-only architecture.
opencode github run;agentinput effective throughdefault_agent, while command frontmatter remains authoritative;event: COMMENT;/review-pr simplifyas proposal-only review suggestions that never edit files.Deliberately out of scope
This PR does not attempt to resolve #24 and does not include:
Validation
agentselection, JSONC merging, review target/payload validation, and stale-head rejection;