redhat-et / redhat-et/ProtoBot
Fix two unresolved Medium findings from PR #124's final review round: guard rule 6 init-branch lifetime gap and OpenCode native wildcard bypassing content-file/impact-file restriction
Nobody has claimed this yet.
- Dominant language
- Go
- Stars
- 5
- Forks
- 6
- Avg merge
- 23h 24m
- Merged PRs (30d)
- 66
Description
What happened
PR #124 (https://github.com/redhat-et/ProtoBot/pull/124) went through 4 automated fullsend-ai-review rounds. The final round (workflow run https://github.com/redhat-et/ProtoBot/actions/runs/35204502556, posted 2026-09-17T09:43:05Z) reviewed commit 6cc9a55 — the exact commit that was merged, with no further commits after — and posted 3 Medium + 1 Low findings as unresolved inline review comments. None were replied to, fixed, or tracked; the PR merged 2026-09-17T16:08:56Z (~6h15m later) via a bare 'APPROVED' from JohnStrunk with no comments addressing them. I read the merged files directly (not just the bot's claims) and confirmed two of the Medium findings are real: (a) docs/architecture/agent-harness/adapter-contract.md guard rule 6 (lines 631-640) only checks that a shell command's ////<title> placeholders match recorded project state; it does not check whether .protobot/ already existed, so the initialization-branch form git switch -c <prefix>00001-project-init <default> still matches rule 6 after setup is complete, even though the shell-operations table (line 406) and guard rule 1 (lines 581-593) describe that form as valid only before .protobot/ exists — the enforcement mechanism (rule 6) doesn't encode the constraint the prose asserts. (b) docs/architecture/agent-harness/opencode.md's native bash-rule block (line 196) is "ears-manager *": allow with no restriction on flag values, while adapter-contract.md (lines 324-428) states the core guard refuses --content-file/--impact-file with any value but -, and even ships a guard-vector fixture for --content-file ~/.netrc (line 1163, a credential-exfiltration vector) — but nothing in opencode.md's native layer mirrors that restriction, so if OpenCode's guard hook were ever skipped (a fail-open case the adapter-contract explicitly discusses elsewhere), the native wildcard alone would let the credential-exfiltration form through. This is the same failure pattern as PR #123, where retro filed issue #129 (closed) for four leftover findings that also went unaddressed at merge — i.e. this is a recurring pattern, not a one-off.
What could go better
High confidence on findings (a) and (b): I traced the cross-references myself across adapter-contract.md, opencode.md, and the shell-operations table rather than trusting the bot's summary alone, and both describe a genuine inconsistency between stated intent and the literal enforcement text. Lower confidence on the other two findings from that same round (a claude-code.md fail-open nuance on shell exit 126/127, and an 'cannot change what the agent can do' overstated-claim finding in adapter-contract.md): reading claude-code.md lines 127-132 in the merged file, the fail-open condition for exit 126/127 already appears explicitly documented ('a shim the shell cannot run (status 126 or 127 ...) yields no status 2 ... H8 records the gap'), so that finding may already be substantially addressed or may be a narrower re-flag of already-acknowledged behavior — I'd defer to a human or implementer to confirm rather than asserting it's unresolved. This PR is explicitly a design/spec document that defers full implementation-proving to issue #33/#77, and several other findings across earlier rounds were legitimately closed as 'recorded gap, no change requested' — but (a) and (b) describe the guard/native-rule mechanics contradicting their own stated guarantees, which is a different class of problem than an acknowledged future gap.
Proposed change
Open a follow-up PR against redhat-et/ProtoBot's docs/architecture/agent-harness/ documents that: (1) in adapter-contract.md guard rule 6 (~line 631), add an explicit check that the initialization form is refused whenever .protobot/ already exists (not merely implied by rule 1's prose), and add a guard test vector that runs git switch -c cs/00001-project-init main against a fixture clone where .protobot/ exists and expects a refusal; (2) in opencode.md's native bash-rule block (~line 196), replace the unconditional "ears-manager *": allow with a rule that denies --content-file/--impact-file when followed by anything other than -, or, if OpenCode's rule syntax can't express that, explicitly document (matching the style used elsewhere for Claude Code/Codex gaps) that this specific restriction relies solely on the guard hook and the native layer provides no defense-in-depth here; (3) explicitly confirm and close out, one way or the other, whether the claude-code.md fail-open finding and the adapter-contract.md overstated-claim finding are already covered by the existing 'H8 records the gap' language.
Validation criteria
A follow-up PR addressing points 1-2 merges, and the next fullsend-ai-review pass on that PR does not re-raise the guard-rule-6 lifetime-constraint or the opencode.md permission-expansion finding. When issue #77 (OpenCode Drafting Table MVP) is implemented, its guard fixture includes both new test vectors described above.
Generated by retro agent from https://github.com/redhat-et/ProtoBot/pull/124
Contributor guide
No contributing guide indexed for this repository
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Read the guard rule 6 and guard-vector sections in docs/architecture/agent-harness/adapter-contract.md, then compare the native bash-rule block in docs/architecture/agent-harness/opencode.md with the shell-operations table. Add the initialization-lifetime vector and address the content-file/impact-file restriction or document the native-layer gap; confirm the two lower-confidence findings against the existing claude-code.md wording. Done means the follow-up review no longer raises the two confirmed findings.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- git, markdown
- Domain
- documentation, security
- Issue type
- Documentation
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Active
- Clarity
- Mostly clear
- Newbie friendliness
- 55/100