Skip to content

fix: scope provider gatekeeper suppressions and declare honest lexical scope - #2702

Merged
steipete merged 1 commit into
mainfrom
steipete/gatekeeper-scope-final
Aug 6, 2026
Merged

fix: scope provider gatekeeper suppressions and declare honest lexical scope#2702
steipete merged 1 commit into
mainfrom
steipete/gatekeeper-scope-final

Conversation

@steipete

@steipete steipete commented Aug 6, 2026

Copy link
Copy Markdown
Owner

Final slice of the provider-architecture campaign: fixes the two remaining gatekeeper suppression-precision bugs (argument-scoped log suppression; category: limited to log-category constructors), each with exact regressions, and replaces the aspirational threat-model wording with the honest lexical-scanner scope — what is detected, what is documented out of scope and why, and the SwiftSyntax-based implementation named as the upgrade path if real drift ever slips the tripwire.

Proof: gatekeeper 34/34 zero findings, make check clean, full suite 821/821, Linux target builds, autoreview clean.

🤖 Generated with Claude Code

@clawsweeper clawsweeper Bot added rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. P3 Low-risk cleanup, docs, polish, ergonomics, or speculative feature. merge-risk: 🚨 other 🚨 Merging this PR has meaningful risk outside the owned taxonomy. labels Aug 6, 2026
@clawsweeper

clawsweeper Bot commented Aug 6, 2026

Copy link
Copy Markdown

Codex review: needs changes before merge. Reviewed August 6, 2026, 2:44 AM ET / 06:44 UTC.

ClawSweeper review

What this changes

The PR narrows provider-gatekeeper suppressions, adds parser regressions, and documents the lexical scanner’s actual scope.

Merge readiness

⚠️ Ready for maintainer review - 3 items remain

Keep open: the new logging exemption still suppresses a provider literal when it appears inside a nested policy call passed to a logger, so the gatekeeper can miss an in-scope architecture violation.

Priority: P3
Reviewed head: 04b7f275322ce495bb8d19483439fe8dc56f47cd

Review scores

Measure Result What it means
Overall readiness 🦐 gold shrimp (3/6) The patch is focused and documented, but one concrete suppression-boundary defect must be repaired before merge.
Proof confidence 🌊 off-meta tidepool Not applicable: This owner-authored lexical test and documentation change is appropriately evaluated through focused parser regressions and repository checks rather than a user-facing runtime demonstration.
Patch quality 🦐 gold shrimp (3/6) 1 actionable review finding remain.

Verification

Check Result Evidence
Real behavior Not applicable Not applicable: This owner-authored lexical test and documentation change is appropriately evaluated through focused parser regressions and repository checks rather than a user-facing runtime demonstration.
Evidence reviewed 5 items Nested logging scope defect: The new logging check returns true when any unmatched enclosing call is a logger. A literal in a nested policy call such as logger.info("...", metadata: ProviderRule(provider: "claude")) is therefore suppressed even though it is not log text.
New coverage does not exercise nesting: The added regression checks a top-level log message and separate provider-selection calls, but not a provider-policy argument nested under a logging call.
Documented contract: The changed guide explicitly says raw provider-ID function-argument policy positions are in scope, which includes the nested policy argument missed by the implementation.
Findings 1 actionable finding [P2] Limit log suppression to direct logging arguments
Security None None.

How this fits together

CodexBar centralizes provider identifiers and policies across the app, CLI, and provider integrations. Its provider-architecture gatekeeper is a test-only lexical scanner over shipped Swift that reports forbidden provider-specific references during validation.

flowchart LR
A[Shipped Swift source] --> B[Lexical gatekeeper]
B --> C[Provider token scan]
C --> D[Scoped suppressions]
D --> E[Gatekeeper findings]
E --> F[CI validation]
G[Provider architecture guide] --> B
Loading

Before merge

  • Limit log suppression to direct logging arguments (P2) - openingParentheses.reversed().contains accepts any enclosing logger call. Thus logger.info("…", metadata: ProviderRule(provider: "claude")) suppresses the inner policy literal, even though function-argument policy positions are documented as in scope. Restrict this to the direct logging argument and add that nested regression.
  • Resolve merge risk (P1) - Merging as-is leaves the drift tripwire blind to provider-policy literals nested inside a logging call, despite the documented function-argument coverage.
  • Complete next step (P2) - A narrow mechanical repair and regression test can address the only blocking finding; this owner-authored PR remains open for normal review.

Findings

  • [P2] Limit log suppression to direct logging arguments — Tests/CodexBarTests/ProviderArchitectureGatekeeperTests.swift:3906-3910
Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Changed surface 2 files; tests +211/-27, docs +15/-7 The patch is confined to the lexical guard and its contract documentation, with no production runtime code changed.

Merge-risk options

Maintainer options:

  1. Narrow nested log suppression (recommended)
    Restrict the log exemption to the direct logging argument and add a regression proving nested provider-policy literals remain findings.
  2. Pause the lexical-scope expansion
    Pause this branch if maintainers prefer to retain the earlier broader suppression until parser-backed scoping is available.
Copy recommended automerge instruction
@clawsweeper automerge

Special instructions:
Restrict log-literal suppression to direct logging arguments, add a nested-policy regression, and run the focused gatekeeper test plus repository checks.

Technical review

Best possible solution:

Suppress only a literal that belongs directly to a recognized logging call, while retaining detection for nested policy expressions and adding a focused regression for that boundary.

Do we have a high-confidence way to reproduce the issue?

Yes—source inspection gives a high-confidence path: place a provider-policy literal in a nested call within a logging argument and the new ancestor-call check suppresses it.

Is this the best way to solve the issue?

No. The intended token-level solution needs direct-call scoping; checking every enclosing logging call is broader than the documented lexical contract.

Full review comments:

  • [P2] Limit log suppression to direct logging arguments — Tests/CodexBarTests/ProviderArchitectureGatekeeperTests.swift:3906-3910
    openingParentheses.reversed().contains accepts any enclosing logger call. Thus logger.info("…", metadata: ProviderRule(provider: "claude")) suppresses the inner policy literal, even though function-argument policy positions are documented as in scope. Restrict this to the direct logging argument and add that nested regression.
    Confidence: 0.98

Overall correctness: patch is incorrect
Overall confidence: 0.98

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against 1ff063fc3158.

Labels

Label justifications:

  • P3: This is a focused internal validation and documentation repair with no direct user-facing runtime change.
  • merge-risk: 🚨 other: The changed validation guard can silently miss an in-scope provider-policy violation after merge.
  • rating: 🦐 gold shrimp: Overall readiness is 🦐 gold shrimp; proof is 🌊 off-meta tidepool and patch quality is 🦐 gold shrimp.
  • status: ⏳ waiting on author: ClawSweeper has contributor-facing work open and is waiting for author action. Not applicable: This owner-authored lexical test and documentation change is appropriately evaluated through focused parser regressions and repository checks rather than a user-facing runtime demonstration.

Evidence

Acceptance criteria:

  • [P1] swift test --filter ProviderArchitectureGatekeeperTests.
  • [P1] make check.
  • [P1] make test.

What I checked:

Likely related people:

  • steipete: Current main’s recent gatekeeper hardening series and this branch are authored by steipete/Peter Steinberger. (role: recent area contributor; confidence: high; commits: 1ff063fc3158, d96e1822f856, 04b7f275322c; files: Tests/CodexBarTests/ProviderArchitectureGatekeeperTests.swift, docs/provider.md)

Rank-up moves

Optional improvements that raise the rating; they are not merge blockers.

  • Add a regression for a provider-policy literal nested inside logger metadata.
  • Restrict the exemption to the literal’s direct logging-call argument and rerun the focused gatekeeper test and repository checks.

Rating scale

Score Internal tier Crab rank Meaning
6/6 S 🦀 challenger crab Exceptional readiness
5/6 A 🦞 diamond lobster Very strong readiness
4/6 B 🐚 platinum hermit Good normal PR; ordinary maintainer review
3/6 C 🦐 gold shrimp Useful, but confidence is limited
2/6 D 🦪 silver shellfish Proof or implementation needs work
1/6 F 🧂 unranked krab Not merge-ready
N/A NA 🌊 off-meta tidepool Rating does not apply

Overall follows the weaker of proof and patch quality.
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

Workflow

  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

@steipete
steipete merged commit fb88548 into main Aug 6, 2026
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge-risk: 🚨 other 🚨 Merging this PR has meaningful risk outside the owned taxonomy. P3 Low-risk cleanup, docs, polish, ergonomics, or speculative feature. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant