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

Open
#139 2 comments 1 reaction 0 assignees View on GitHub

Nobody has claimed this yet.

component:specification-toolkit documentation ready-for-triage triaged
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

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.