patterns: drop sandbox/scripts wholesale block, enforce via reviewer_security (#2133) by jwbron · Pull Request #2135 · jwbron/egg

jwbron · 2026-04-27T04:56:22Z

Summary

Closes #2133.

The wholesale block on sandbox/scripts/ in CODER_PATTERNS was defense-in-depth, not a runtime control. The gateway sidecar is the sole egress chokepoint: the sandbox container holds no credentials, and any request a wrapper emits is independently re-validated by the gateway against its policy. The only effect of the wholesale block was to force a two-step dance for every new wrapper (#1556 jira, #1931 confluence) — stage at .egg-state/agent-outputs/, exempt in patterns.py, deploy, then git mv post-merge — because the gateway enforces the patterns.py it loaded at startup, not the one the coder just edited.

Rather than plumbing hot-reload around a control that wasn't load-bearing, this drops the wholesale block entirely and moves the enforcement to reviewer_security, where a wrapper-shape change can be evaluated as the trust-boundary diff it is.

Changes

shared/egg_restrictions/patterns.py — remove "sandbox/scripts/" from CODER_PATTERNS.blocked_patterns and drop the now-redundant sandbox/scripts/jira block_exempt entry.
shared/prompts/security-review-criteria.md — add a new priority-5 section "Credential-shim modifications under sandbox/scripts/" covering the actual threat surface a compromised wrapper exposes (misleading the caller, smuggling fields into forwarded request bodies, route confusion, session-token exfiltration through allowed requests). Marked as a blocking finding for any deviation from the documented wrapper shape. Also adds sandbox/scripts/ to the trust-boundary list in "How to Review". The OWASP section is renumbered 6 → 7.
Tests (shared/tests/test_egg_restrictions.py, gateway/tests/test_agent_restrictions_patterns.py, gateway/tests/test_agent_restrictions_comprehensive.py) — flip the four test_blocks_sandbox_scripts_* assertions to test_allows_*, with comments pointing at the security reviewer.
Docs (docs/reference/agent-roles.md, docs/guides/sdlc-pipeline.md) — reflect the policy change and link to the security-reviewer criteria.

Test plan

pytest shared/tests/test_egg_restrictions.py gateway/tests/test_agent_restrictions_patterns.py gateway/tests/test_agent_restrictions_comprehensive.py (570 passed)
pytest gateway/tests/test_phase_filter.py gateway/tests/test_phase_filter_restrictions.py gateway/tests/test_partition_files_by_role.py orchestrator/tests/test_pipeline_prompts.py (503 passed) — adjacent suites that consume blocked_patterns / block_exempt_patterns unchanged.
CI green
Smoke: a future pipeline that adds a new sandbox/scripts/<wrapper> should land it directly in the canonical path with no git mv follow-up obligation.

Out of scope

The git mv cleanup for Add Confluence gateway support (read-only v1) #1931's staged .egg-state/agent-outputs/1931-sandbox-scripts-confluence — lives in the Add Confluence gateway support (read-only v1) #1931 PR follow-through.
The tests/sandbox/test_jira_wrapper.py artifact-fallback (_ARTIFACT = .egg-state/agent-outputs/1556-sandbox-scripts-jira) is left in place; it harmlessly prefers the canonical path when present and is workaround scaffolding for the same problem this PR fixes — can be cleaned up separately.

…eviewer_security (#2133) The wholesale block on `sandbox/scripts/` in CODER_PATTERNS was defense-in-depth, not a runtime control: the gateway sidecar is the sole egress chokepoint, the sandbox container holds no credentials, and any request a wrapper emits is independently re-validated by the gateway. The block's only effect was to force a two-step dance for new wrappers (#1556 jira, #1931 confluence) — stage at .egg-state/agent-outputs/, exempt in patterns.py, deploy, then git mv post-merge — because the gateway sidecar enforces the patterns.py it loaded at startup, not the one the coder just edited. Changes: - Remove "sandbox/scripts/" from CODER_PATTERNS.blocked_patterns and drop the now-redundant sandbox/scripts/jira block_exempt entry. - Update the four pattern tests that asserted on the wholesale block to assert it is now writable, with rationale comments pointing at the security reviewer. - Add a "Credential-shim modifications under sandbox/scripts/" section to security-review-criteria.md (priority slot 5) covering the actual threat surface a compromised wrapper exposes (misleading the caller, smuggling fields into request bodies, route confusion, session-token exfiltration through allowed requests). Marked as a blocking finding for any deviation from the documented wrapper shape. Also adds sandbox/scripts/ to the trust-boundary list in "How to Review". - Update docs/reference/agent-roles.md and docs/guides/sdlc-pipeline.md to reflect the new policy.

egg-reviewer

No agent-mode design concerns.

The change actually moves toward agent-mode alignment: it drops a defense-in-depth block at the role-pattern layer that wasn't load-bearing (the gateway is the real sandbox-enforced control) and was creating workflow friction (two-step dance for every new wrapper). The security-review-criteria.md addition is a reviewer checklist for trust-boundary diffs, which is the right level — not prompt-level enforcement of a sandbox-enforceable invariant.

— Authored by egg

egg-reviewer

Verdict: Request changes

The PR's stated goal is to drop the wholesale block on sandbox/scripts/ so a future pipeline can land a new wrapper "directly in the canonical path with no git mv follow-up obligation." That goal is not achieved by this diff, and the new security-review prose makes a factual claim about the system that is currently untrue. There are also two design points worth surfacing before this lands.

Blocking — `.egg/phase-permissions.json` still blocks `sandbox/scripts/` for the coder role

shared/egg_restrictions/patterns.py::CODER_PATTERNS is not the only coder write filter the gateway runs on a push. The gateway also calls check_file_restrictions(session_role, changed_files) at gateway/gateway.py:1247, which walks PhaseFilter._file_restrictions loaded from .egg/phase-permissions.json. That file still has:

{
  "role": "coder",
  "blocked_patterns": [
    ...
    ".github/",
    "sandbox/scripts/"     // ← still here
  ],
  "blocked_reason": "Coder cannot modify tests, docs, pipeline-state, CI workflows, or credential shims; see #1901.",
  "description": "TODO(#1903): keep in sync with CODER_PATTERNS / TESTER_PATTERNS / DOCUMENTER_PATTERNS in shared/egg_restrictions/patterns.py..."
}

FileRestriction.is_file_blocked (gateway/phase_filter.py:80-94) is startswith matching with no block_exempt analogue, so a coder push of sandbox/scripts/gh will be rejected by the gateway with Push denied: Role 'coder' cannot modify: sandbox/scripts/gh regardless of the patterns.py change. The two-step .egg-state/agent-outputs/ → human git mv dance the PR description targets is still required.

The TODO comment on the entry literally says "keep in sync with CODER_PATTERNS" — this synchronization step was missed. The PR also adds new text to shared/prompts/security-review-criteria.md (line 116-118) asserting:

"The role-level write filter does not block writes under sandbox/scripts/ — the credential-routing invariant is enforced by this lens, not by patterns.py."

That statement is currently false at the gateway level. It will be true only after this file is updated.

Fix: remove "sandbox/scripts/" from .egg/phase-permissions.json file_restrictions[role=coder].blocked_patterns, update the blocked_reason and description strings to match patterns.py, and add a test_coder_allowed_for_sandbox_scripts case to TestThreeRoleFileRestrictions in gateway/tests/test_phase_filter_restrictions.py to lock the synchronization in.

Blocking — the threat-model claim that "compromise is enforced by `reviewer_security`" is weaker than the diff implies

reviewer_security is wired into the implement-phase reviewer roster (shared/egg_contracts/agent_roles.py:1114-1121) and the criteria file is loaded by _get_security_review_criteria() (orchestrator/routes/pipelines.py:3464-3499), so on the orchestrator-driven path the lens does run. But:

The lens is ReviewCriticality.ADVISORY per orchestrator/review_graph.py:215-263 and the criteria file itself (line 14-16): "your NACKs are recorded on the approval matrix but do not deadlock consensus." A producer can confirm while reviewer_security is in NACK. So this PR replaces a hard write-block with a soft signal that the producer can ignore.
PRs created without an orchestrator pipeline (humans, babysit_pr is implement-phase BRC only — direct human-authored PRs are not) never instantiate reviewer_security at all. The wholesale block was a pre-merge guard that fired regardless of who created the PR; the new posture only fires when an orchestrator-driven implement BRC happens to be running.

This is a substantive weakening. The PR description argues the gateway is the load-bearing control because "the sandbox container holds no credentials" — fair, and section 5 of the new criteria correctly enumerates the residual threats (misleading-output / smuggle-into-body / route-confusion / session-token exfil through allowed routes). But the residual threats are exactly the kind a hard merge-gate exists to catch when an attacker-controlled diff lands in the credential-shim trust boundary. The PR description should either:

Acknowledge that reviewer_security is advisory and explain why advisory-only is sufficient for this trust boundary, or
Promote reviewer_security to CRITICAL for diffs touching sandbox/scripts/* (the existing tracker is #1997). A scoped promotion — "advisory in general, critical when sandbox/scripts/* is in the diff" — would actually match the threat model the new criteria describe.

As written, the diff trades a deterministic block for an advisory note that the producer can override and that doesn't fire on human PRs at all.

Non-blocking — internal contradiction in section 5 of the criteria

Lines 14-16 say the lens is advisory ("NACKs do not deadlock consensus"). Line 138-139 then says "This is a blocking finding regardless of code quality: any deviation from the documented wrapper shape is a NACK." Those two statements are inconsistent — within an advisory lens, no finding can actually block. Recommend rephrasing to something like:

This is a mandatory NACK (do not silently approve any deviation from the documented wrapper shape). Note that the lens is advisory today, so the NACK is recorded as a finding rather than a deadlock — promotion to critical for sandbox/scripts/* diffs is tracked in #1997.

That removes the contradiction and makes the actual posture explicit to the reviewer agent loading this prompt.

Non-blocking — PR description overstates what the diff does

"this drops the wholesale block entirely"

It doesn't — it drops one of two layers. Either fix the second layer or rephrase to "drops the wholesale block at the patterns.py layer." The description as written will mislead the merger and any future reader auditing why the credential-routing invariant changed shape.

What I checked

shared/egg_restrictions/patterns.py change: clean removal of the sandbox/scripts/ blocked pattern and the sandbox/scripts/jira block-exempt. No references to the removed exemption remain in the test surface (grep -rn sandbox_scripts_jira is empty after the diff).
Test flips in gateway/tests/test_agent_restrictions_patterns.py, gateway/tests/test_agent_restrictions_comprehensive.py, and shared/tests/test_egg_restrictions.py are symmetric — block-list and allow-list tables are updated together, no orphaned test names.
Section renumbering 5→6, 6→7 in security-review-criteria.md: no external file cites these by number (the only "Section 5/6/7" hits in orchestrator/tests/test_removal_validation_1165.py and test_producer_push_consensus.py are about unrelated test-file headers).
Doc updates in docs/guides/sdlc-pipeline.md and docs/reference/agent-roles.md correctly reflect the new posture.
reviewer_security activation: confirmed it ships as part of the implement-phase BRC roster on every repo (not gated to egg-only), and the criteria file is loaded as the security-lens system prompt.

— Authored by egg

…tradiction - .egg/phase-permissions.json: remove sandbox/scripts/ from coder file_restrictions blocked_patterns. The patterns.py change in this PR is not load-bearing alone — the gateway also enforces this JSON file via PhaseFilter._file_restrictions / check_file_restrictions, which uses startswith matching. Without this sync, a coder push of sandbox/scripts/<wrapper> still gets denied by the gateway with Push denied: Role 'coder' cannot modify: sandbox/scripts/<...>, defeating the PR's goal. Updates blocked_reason and description to match patterns.py and to point reviewers at the security lens. - gateway/tests/test_phase_filter_restrictions.py: add test_coder_allowed_for_sandbox_scripts under TestThreeRoleFileRestrictions to lock the cross-layer synchronization between .egg/phase-permissions.json and shared/egg_restrictions/patterns.py. - shared/prompts/security-review-criteria.md: rephrase the section 5 closing paragraph that contradicted the advisory-lens scope at the top of the file. The new wording calls deviations a 'mandatory NACK' (the reviewer must record the finding) while making the advisory posture explicit and pointing at #1997 as the tracker for promotion to a critical (deadlock-capable) lens for sandbox/scripts/* diffs.

james-in-a-box · 2026-04-27T05:13:22Z

Thanks for the careful review. Addressed in 2929e3d:

Blocking — .egg/phase-permissions.json sync: Agreed, this was the load-bearing miss. Removed "sandbox/scripts/" from file_restrictions[role=coder].blocked_patterns, updated blocked_reason and description strings to match patterns.py and point at the security lens, and added test_coder_allowed_for_sandbox_scripts to TestThreeRoleFileRestrictions in gateway/tests/test_phase_filter_restrictions.py to lock the cross-layer synchronization in. The PR's stated goal — landing a new wrapper directly in the canonical path with no git mv dance — is now actually achieved at both gateway layers.

Non-blocking — section 5 contradiction: Agreed. Rephrased the closing paragraph (shared/prompts/security-review-criteria.md) to call deviations a mandatory NACK (the reviewer must record the finding) while making the advisory-lens posture explicit per the file's Scope heading at the top, and pointing at #1997 as the tracker for promotion to a critical (deadlock-capable) lens for sandbox/scripts/* diffs.

Blocking — threat-model claim weaker than the diff implies: Agreed on the substance. The trade-off is intentional for this iteration — the credential-routing invariant is enforced at runtime by the gateway on every request regardless of wrapper shape, so the residual threats (mislead, smuggle, route-confuse, exfil) are the only things a wrapper-shape review actually catches, and an advisory recording on orchestrator-driven PRs is sufficient to surface a regression to a human merger. Promotion to critical scoped to sandbox/scripts/* diffs is the right next step and lives in #1997. I cannot edit the PR description (it's authored by jwbron, gh blocks the edit), so leaving this comment as the on-record acknowledgement; happy to defer the merge until the description is updated by someone who can edit it, or to file a follow-up PR that promotes reviewer_security for sandbox/scripts/* paths against #1997 if that's the preferred resolution path.

Non-blocking — PR description overstates the diff: Same constraint — flagged via this comment rather than an edit. The 2929e3d commit message and the new tests make the actual posture (both layers dropped, advisory recording + runtime gateway re-validation, #1997 for the deadlock promotion) explicit on-tree.

— Authored by egg

egg-reviewer

No agent-mode design concerns in the delta since 462526f.

The new commit (2929e3d) addresses general-review feedback only:

.egg/phase-permissions.json sync — removes sandbox/scripts/ from coder blocked_patterns to match patterns.py. This is a config-sync fix, not an agent-mode design change.
gateway/tests/test_phase_filter_restrictions.py — adds test_coder_allowed_for_sandbox_scripts to lock the cross-layer synchronization. Standard regression test.
shared/prompts/security-review-criteria.md — rephrases section 5 closing paragraph to call deviations a mandatory NACK (reviewer must record the finding) while making the advisory-lens posture explicit and pointing at #1997 for promotion to a critical lens.

The reviewer-prompt rephrasing remains well-aligned with agent-mode principles: it states verifiable objectives (wrapper shape, route mapping, output faithfulness, new-wrapper checks) rather than micromanaging step-by-step procedures, and the prior approval's reasoning still holds — this is a reviewer checklist for trust-boundary diffs at the right level, not prompt-level enforcement of a sandbox-enforceable invariant.

— Authored by egg

egg-reviewer

Verdict: Approve

The follow-up commit 2929e3d resolves both blocking findings from my prior review and the non-blocking section-5 contradiction. The PR's stated goal — landing a new wrapper directly under sandbox/scripts/ with no git mv follow-up — is now actually achievable through both the patterns.py and .egg/phase-permissions.json layers.

Verification of fixes

.egg/phase-permissions.json synchronized. "sandbox/scripts/" removed from file_restrictions[role=coder].blocked_patterns (/.egg/phase-permissions.json:194-218). blocked_reason and description strings now point at reviewer_security and the security-review criteria file. I traced the path: gateway/gateway.py:1247 → check_file_restrictions → FileRestriction.is_file_blocked (gateway/phase_filter.py:80-94). With sandbox/scripts/ gone from this list, a coder push of sandbox/scripts/<wrapper> is no longer rejected. I confirmed by running pytest gateway/tests/test_phase_filter_restrictions.py shared/tests/test_egg_restrictions.py gateway/tests/test_agent_restrictions_patterns.py gateway/tests/test_agent_restrictions_comprehensive.py (653 passed).
Sync-locking test added. test_coder_allowed_for_sandbox_scripts at gateway/tests/test_phase_filter_restrictions.py:590-598 exercises the gateway's actual check_file_restrictions("coder", ["sandbox/scripts/gh"]) path and asserts allowed is True. The docstring explicitly cites the patterns.py / phase-permissions.json drift hazard, which is the right framing — future re-additions of the block will trip this test alongside the existing test_allows_sandbox_scripts_* cases at the patterns.py layer.
Section 5 contradiction resolved. shared/prompts/security-review-criteria.md:138-144 now says mandatory NACK (the reviewer must record the finding) while making the advisory-lens posture explicit and pointing at #1997 as the deadlock-promotion tracker. The phrasing matches the Scope heading at the top of the file (lines 14-16) and removes the internal inconsistency.

Acknowledged trade-offs

Threat-model weakening (advisory-only on orchestrator-driven PRs, no lens at all on direct human PRs). Producer acknowledged this is intentional for this iteration in the PR comment thread, with #1997 tracking promotion to a CRITICAL lens scoped to sandbox/scripts/* diffs. The argument that the gateway is the load-bearing runtime control because the sandbox holds no credentials is sound; the residual threats (mislead, smuggle, route-confuse, exfil-via-allowed-route) are exactly what a wrapper-shape review catches and an advisory recording is sufficient to surface them to a human merger. I'm comfortable shipping this with #1997 as the on-record follow-up.
PR description still says "drops the wholesale block entirely" which doesn't reflect the two-layer scope — but the producer cannot edit the description (it was authored by jwbron) and the commit message + tests make the actual posture clear on-tree. Non-blocking.

Other things I checked

phase_file_restrictions.implement.blocked_patterns (/.egg/phase-permissions.json:170-178) only blocks .egg-state/... paths — does not interfere with sandbox/scripts/.
shared/egg_container/__init__.py::_IMPLEMENT_READONLY_DIRS (shared/egg_container/__init__.py:136) is ("drafts", "contracts", "pipelines", "reviews") — does not include sandbox/scripts/, so the readonly-mount mechanism doesn't block this either.
No other coder-scoped layer references sandbox/scripts/ as a block. TESTER_PATTERNS and DOCUMENTER_PATTERNS correctly leave it alone (it was only ever a coder-block). Doc references in docs/reference/agent-roles.md:166-169 and docs/guides/sdlc-pipeline.md reflect the new posture.
Tests pass cleanly: 653 passed across the four restriction-related suites.

Non-blocking suggestion

test_coder_allowed_for_sandbox_scripts only covers sandbox/scripts/gh. The patterns.py test layer covers both gh and git-credential-github-token (gateway/tests/test_agent_restrictions_patterns.py:362-370, shared/tests/test_egg_restrictions.py:219-227) — for symmetry, consider adding a sandbox/scripts/git-credential-github-token case to the new gateway-layer test as well. The existing one is sufficient to lock in the synchronization, so this is purely a defense-in-depth ask.

— Authored by egg

Defense-in-depth follow-up to review feedback on PR #2135. Mirrors the patterns.py test layer (which covers both gh and git-credential-github-token) so the gateway-layer synchronization test locks in coverage of both wrappers a coder might land under sandbox/scripts/.

james-in-a-box

Thanks for the approval. Addressed the non-blocking defense-in-depth suggestion in a8d1d83: extended test_coder_allowed_for_sandbox_scripts to also exercise sandbox/scripts/git-credential-github-token, mirroring the patterns.py-layer coverage (gateway/tests/test_agent_restrictions_patterns.py:362-370, shared/tests/test_egg_restrictions.py:219-227). Both wrappers now lock in synchronization at both the patterns.py and .egg/phase-permissions.json layers.

— Authored by egg

james-in-a-box · 2026-04-27T05:26:51Z

Autofix tracking

{"Lint/Python": 1}

egg-reviewer

No agent-mode design concerns in the delta since 2929e3d.

The two new commits are mechanical:

a8d1d83 — Extends test_coder_allowed_for_sandbox_scripts to also exercise sandbox/scripts/git-credential-github-token, mirroring the patterns.py-layer coverage. This addresses my prior non-blocking defense-in-depth suggestion and locks in the cross-layer synchronization for both wrappers.
dccf9de — Automated formatting fix (collapses the multi-line check_file_restrictions(...) call onto one line). No semantic change.

Neither touches prompts, agent orchestration, or sandbox/gateway boundaries — both are test-only changes to lock cross-layer synchronization. Prior approval reasoning still holds.

— Authored by egg

egg-reviewer

Verdict: Approve

The delta since 2929e3d (commits a8d1d83 + dccf9de) is exactly the non-blocking defense-in-depth suggestion from my prior review, applied as asked.

Verification of the delta

test_coder_allowed_for_sandbox_scripts extended to cover the second wrapper. gateway/tests/test_phase_filter_restrictions.py:600 now also exercises check_file_restrictions("coder", ["sandbox/scripts/git-credential-github-token"]) and asserts allowed is True. This mirrors the patterns.py-layer coverage at gateway/tests/test_agent_restrictions_patterns.py:362-370 and shared/tests/test_egg_restrictions.py:219-227, so both wrappers are now sync-locked across both gateway layers.
Docstring updated at gateway/tests/test_phase_filter_restrictions.py:596-597 to explicitly call out the gh + git-credential-github-token parity. The drift-hazard framing from the prior review is preserved.
Formatting fix (dccf9de) is purely cosmetic. Collapses the multi-line check_file_restrictions(...) call into a single line. The result is 98 chars vs the project's line-length = 100 (pyproject.toml) — under the limit, so ruff format correctly inlined it. No semantic change.

Other things I checked

No other files changed since 2929e3d (gh api .../compare returned a single-file delta).
CI on the new tip: Unit Tests, Security Scan, Lint (Python/Shell/YAML/Docker/Actions/Custom), and Aggregate Test Results all green at dccf9de. The two in-flight egg-review/* checks are this review job itself.
The added test path is a pure positive assertion — no risk of false-pass: check_file_restrictions returns allowed=False with blocked_files populated when the path matches a blocked_patterns entry, so a regression that re-adds sandbox/scripts/ to the JSON list will trip both assert result.allowed is True lines.
All four prior-review concerns remain resolved: .egg/phase-permissions.json synchronized, sync-locking test in place (now covering both wrappers), section-5 contradiction rephrased to mandatory NACK with #1997 noted as the deadlock-promotion tracker, and the threat-model trade-off acknowledged on-record.

Nothing else found. Ship it.

— Authored by egg

james-in-a-box · 2026-04-27T05:37:23Z

egg review completed. View run logs

10 previous review(s) hidden.

Phase 3 — sandbox/scripts/confluence (Task 3-1): Bash CLI wrapper that routes Confluence commands through the gateway sidecar. Mirrors sandbox/scripts/jira shape: - Fail-closed on missing gateway sidecar. - EGG_SESSION_TOKEN Bearer auth on every call. - JSON-on-stdout, errors-on-stderr, non-zero exit on non-2xx. Verbs (Jira-style only per Q10): - page get / descendants / footer-comments / inline-comments - space pages / list - search '<CQL>' - execute <METHOD> <PATH> - help Each verb POSTs to the matching /api/v1/confluence/* endpoint where the gateway enforces space allowlist, CQL scope, response redaction, and the read-only fence. The wrapper itself never holds Atlassian credentials — gateway-side credential injection is the single trust boundary. Permitted via shared/egg_restrictions/patterns.py block_exempt_patterns landed in #2133/#2135 — the path 'sandbox/scripts/confluence' is the narrow exemption added alongside the existing 'sandbox/scripts/jira' exemption from #1556.

Two reviewer_code blockers from NACK 2d5b7b98 on commit 8b46dfc: 1. Drop the "Pre-merge obligation: sandbox script staging" section from docs/reference/confluence-wrapper.md. Coder commit 72ee7dc committed sandbox/scripts/confluence at its canonical path; the shared/egg_restrictions/patterns.py block_exempt_patterns landed in #2133/#2135 already, so the staging route is obsolete and the git-mv instructions in the section were stale and would mislead maintainers. 2. Soften the "Anti-bypass invariant" paragraph in the /execute section. The previous wording claimed /execute rejects narrow- route path families (api/v2/pages/{id}, api/v2/spaces/{id}/pages, rest/api/search) and a regression test enforces this; neither is true at branch HEAD — gateway/confluence_client.py:163-175 ships those three patterns IN the /execute allowlist. Replaced with a "Known gap (tracked under issue #1931 cycle-2 NACK)" note that honestly describes the current state and points at the in-flight coder fix. Updated the Error cases table row and the Hard limits line in sandbox/agent-config/rules/environment.md to match. Non-blocking incorporated: spelled out the _USER_PROFILE_SELF_RE regex shape (/api/v\d+/users/) in the response-redaction bullet so future readers know v3+ users endpoints are also covered. Verified each change against gateway/confluence_client.py at the current branch HEAD (72ee7dc). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

…te bypass paths (#1931) Cycle-3 NACKs from reviewer_code (9ae21669) and reviewer_security (ec5985ff) identified three concrete cross-partition bypasses in the /execute path allowlist that exactly match the pattern PR #1964 had to fix in the Jira allowlist (the ``^project$`` / ``search/jql`` exclusions in gateway/jira_client.py). All three were exploitable from inside a private-mode sandbox today and contradicted the explicit "Anti-bypass invariant" promised in docs/reference/confluence-wrapper.md and sandbox/agent-config/rules/environment.md. Blocking #1: ``rest/api/search`` in CONFLUENCE_API_ALLOWED_PATHS lets an agent POST {"method":"GET","path":"rest/api/search","query":{"cql":"text ~ \"secret\""}} through /execute and run arbitrary CQL — bypassing extract_search_spaces() entirely. Fix: drop ``re.compile(r"^rest/api/search$")`` from the allowlist. All CQL must now flow through /api/v1/confluence/search where the static extractor enforces space-scope. Blocking #2: ``api/v2/spaces`` in CONFLUENCE_API_ALLOWED_PATHS lets an agent enumerate the full tenant space catalog via execute_raw, which does NOT apply the allowlist filter that list_spaces does. Defeats decision-11 ("agents cannot enumerate the full tenant space set"). Fix: drop ``re.compile(r"^api/v2/spaces$")`` from the allowlist. Space enumeration must now flow through /api/v1/confluence/space/list which filters to the operator's allowlist. Blocking #3: ``api/v2/footer-comments`` and ``api/v2/inline-comments`` (the flat v2 endpoints) accept ``page-id`` in the query string, but no ``spaceKey`` filter exists at upstream. An agent passes {"path":"api/v2/footer-comments","query":{"spaceKey":"ALLOWED", "page-id":"<NON_ALLOWLISTED_PAGE>"}} — the up-front spaceKey gate sees ``ALLOWED``, Atlassian ignores ``spaceKey`` and returns comments from the non-allowlisted page. The audit log records ``spaceKey=ALLOWED``, masking the exfil. Fix: drop both ``re.compile(r"^api/v2/footer-comments$")`` and ``re.compile(r"^api/v2/inline-comments$")`` from the allowlist. The narrow /api/v1/confluence/page/{footer,inline}-comments routes already cover the agent-facing use case AND correctly fetch the parent page to verify its space (with the cycle-1 fail-closed fix on parent-fetch failure from c4d8fa0). Important: these four paths remain reachable INTERNALLY by ConfluenceClient methods that construct them directly (the include_replies side-call inside get_page_footer_comments and the v2-bug fallback inside get_page_inline_comments) — those paths do NOT go through validate_confluence_api_path. Only the agent-facing /execute escape hatch is closed. Stale-artifact cleanup (reviewer_code_holistic 183e44a0 partial fix): delete the now-redundant staged copy at .egg-state/agent-outputs/1931-sandbox-scripts-confluence. Cycle-3 commit 72ee7dc already pushed the wrapper to its real on-disk path (sandbox/scripts/confluence), making the staging artifact dead weight. gateway/gateway.py /execute route: strip the dead ``requires_space_key`` branch that special-cased the four removed paths. Reaching the post-validation block now implies a page- or space-scoped path family, all of which carry an id inline that the existing post-fetch allowlist check resolves to a spaceKey. Verification: - ``make lint`` passes (ruff check + ruff format check + mypy + shellcheck + custom checks). - ``pytest gateway/tests/test_jira_*.py`` all green (173 tests) — Jira reads / search / execute paths unchanged. Out of scope for this commit (handed back to reviewer_code_holistic): - Aligning patterns.py with main #2135 (drop wholesale ``sandbox/scripts/`` block + drop both ``sandbox/scripts/jira`` and ``sandbox/scripts/confluence`` exemptions) requires a coordinated tester-scope test update on the same branch (the gh / git-credential-github-token test assertions in gateway/tests/test_agent_restrictions_*.py and shared/tests/test_egg_restrictions.py expect the wholesale block to remain — they all flip in main #2135). Those test files are blocked for the coder role (tester scope), so the patterns.py alignment cannot be done unilaterally without breaking the test suite on this branch. Recommend resolving via main merge at PR time, or via a dedicated tester-coder coordinated cycle. - The non-blocking cosmetic / observability findings from the three reviewer NACKs (CQL function-name collision, audit-log gap on 413, redundant ``self.space_cache.put`` skip in get_page, eager spaceId cache population, log-once flag scoping, Jira-loader log spam, heredoc quote on gateway-down message, %-encoded smuggling decoder) are tracked for follow-up tickets — every adversarial probe in the test grid is rejected today.

The fallback in `tests/sandbox/test_jira_wrapper.py` was scaffolding for the pre-#2135 workaround when `sandbox/scripts/` was blocked from agent writes. With #2135 merged, new wrappers land at the canonical path directly and the artifact location is permanently dead code. Drops the docstring paragraph, the `_ARTIFACT` constant, and the fallback branch in `_locate_wrapper()`; simplifies the skip message.

…_restrictions/patterns.py Adopt main's version per #2135 (commit 2f693f3): main dropped the wholesale 'sandbox/scripts/' block from CODER_PATTERNS, making the 'sandbox/scripts/jira' and 'sandbox/scripts/confluence' exemptions no-ops. The Confluence sandbox wrapper added by this PR is now permitted without an explicit exemption (security is enforced via reviewer_security instead).

…nly) (#2141) * Initialize SDLC contract for issue #1931 * refine: analysis for #1931 (Confluence gateway read-only v1) Drafts the refine-phase analysis for issue #1931 — Confluence gateway read-only support, mirroring the Jira gateway pattern from #1556. Captures the Confluence v1/v2 API split, the v2-first hybrid (with v1 CQL search and v1 fallbacks for known v2 comment bugs), the space allowlist + verb allowlist, the private-mode-only restriction, and the shared Atlassian credential strategy with the Jira gateway. All fourteen multiple-choice decisions and ten free-form feedback items are registered against the contract. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * Persist statefiles after refine phase * Persist HITL resolution after refine phase gate * plan(architect): add architecture analysis for #1931 Confluence gateway Mirrors the Jira gateway pattern from #1556 with Confluence-specific adaptations (v2-first / v1-fallback hybrid, CQL static-scope extractor, shared ATLASSIAN_* credential triple). Records all 14 HITL decision resolutions and 10 free-form feedback answers from refine. Includes component breakdown, route surface, data-flow walkthrough, and 4 plan- phase open questions for the task_planner / risk_analyst. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * plan: risk assessment for #1931 (Confluence gateway read-only v1) Plan-phase risk_analyst output: 18 risks across security (6), performance (3), compatibility (2), operational (3), data-privacy (1), external-dependency (1), test-coverage (1), and future-write (1). Severity distribution: 1 high (CQL extractor adversarial coverage), 7 medium (most ride on the #1556 Jira-gateway scaffolding so the implementation surface is well-understood), 10 low. Three risks flagged for human review at implement time: - R1: CQL-extractor parity with the JQL adversarial suite must target CQL-specific grammar (text ~ contains, space.category(), etc.). - R14: attachments / restrictions / permissions permanent-denylist enforcement across both narrow-route and /execute paths. - R15: bot-account effective-access asymmetry (per feedback Q9) — must surface a structured forbidden envelope so agents do not retry. External research covered Atlassian's April 2026 v1 deprecation status (endpoint-specific, CQL search has no v2 successor), the March 2026 points-based rate-limit rollout, and confirmed no public CQL-injection CVE for 2025-2026. Includes a four-level rollback plan (config-only -> route-disable -> credential-revoke -> full-revert) and an 18-item implement-phase checklist that maps each risk to a concrete reviewer-verifiable mitigation. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * plan: add task plan for #1931 (Confluence gateway read-only v1) Decomposes the architecture analysis into a single-PR plan with 6 phases and 28 tasks, mirroring the #1556 Jira-gateway scaffolding. Incorporates all 14 refine HITL decisions (verb-noun URL paths, v2-first hybrid, conservative CQL extractor, body-format=storage default per the operator tweak, shared ATLASSIAN_* triple, per-route private-mode gate, context-filters.yaml allowlist, attachments denylist, no per-pipeline env vars, GET-only /execute) plus the architect's per-verb endpoint pinning, page->space resolution caching, and the risk analyst's R1-R18 mitigations (5 MiB payload cap, descendants depth=1/limit=25 default, bot_account_lacks_read_access reason on 403, confluence_v1_fallback audit, ADF redaction, attachments-denylist case/encoding/nesting tests, route-vs-execute anti-bypass test, prompt-injection caveat in docs). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * plan: address reviewer_plan NACK (#1931) Blocking fixes: - Add TASK-1-5 to update gateway/jira_credentials.py for the same ATLASSIAN_*-preferred / JIRA_* fall-back precedence (risk R11). Add TASK-4-1b extending test_jira_credentials.py with the six-combination matrix. Update PR description and pre-merge note so the shared-credential migration story is portable (operators can drop the legacy JIRA_* block once ATLASSIAN_* is fully populated without silently breaking Jira). - Specify v1-also-404 fall-through behaviour for inline-comment fallback in TASK-1-2: v1 200-empty -> {results: [], used_fallback}; v1 404 -> standard not_found envelope with used_fallback=true. Non-blocking fixes: - Replace "decision E5" with "decision-5" (8 sites). - Move boot-time policy log to confluence_policy.py; client logs only body-format default; credentials log precedence (risk R12). - Add `attempt: 1|2` to confluence_upstream_rate_limited audit shape. - Two-sided spaceId<->spaceKey LRU cache populated by both list_spaces and get_page so /space/pages cold-start avoids double round-trip. - Mention architect Q4 in TASK-1-1 acceptance. - Replace PRIVATE_MODE env-var references with private-mode session language (private mode is g.session_mode, not a process env var). - Note TASK-4-7 must verify existing test name at implement time. - Note show-metrics.md is intentionally untouched. - Add Atlassian rate-limit pool-sharing note to TASK-6-4. - TASK-2-9 audit-event shape mirrors whatever the existing Jira reload emits. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * Persist statefiles after plan phase * docs(implement): add Confluence gateway wrapper docs (#1931) Phase 6 of the read-only Confluence gateway wrapper. New docs/reference/confluence-wrapper.md mirrors the Jira reference and covers the eight `/api/v1/confluence/*` routes, the conservative CQL extractor, the `not_found` envelope, the v1 inline-comment fallback, the response-redaction walker (accountId / emailAddress / user-profile _links.webui), the bot-vs-human access caveat, the prompt-injection caveat, the Atlassian rate-limit runbook (with shared-bot quota-pool note), and the future write-verb extension points. Architecture docs gain a Confluence row in the gateway endpoint table, a Confluence section in credential injection covering the shared `ATLASSIAN_*` precedence and `/wiki` base-URL derivation, and an extended Squid allowlist exclusion paragraph naming Confluence hostnames. The sandbox environment rules document the new `confluence` wrapper verbs and call out that no per-pipeline env var is exported (Confluence is reference material, not a unit of work). docs/index.md adds the wrapper to the reference lookup table. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * gateway: Confluence foundation modules + ATLASSIAN_* credential precedence (#1931) Phase 1 of the Confluence read-only gateway support. Introduces the Confluence-specific building blocks the routes will compose: - gateway/confluence_credentials.py — ATLASSIAN_*/CONFLUENCE_* per-key precedence with /wiki base derivation, mtime-cached, thread-safe. - gateway/confluence_client.py — class-shaped v2-first client with v1 fallbacks for inline-comment 404 and footer-comment nested replies, redaction (accountId / emailAddress / user-profile webui links), payload-size cap (5 MiB), 429 single-retry, 403 envelope, space cache. - gateway/confluence_policy.py — confluence.spaces YAML allowlist loader with mtime cache and fail-closed semantics. - gateway/confluence_search.py — conservative CQL space=/space IN(...) extractor mirroring jira_search's deny-on-ambiguity stance. - gateway/jira_credentials.py — extended to honor ATLASSIAN_*/JIRA_* per-key precedence so the shared-credential migration is portable. Also adds confluence: section to config/context-filters.yaml (empty, fail-closed) and ATLASSIAN_* triple to config/secrets.template.env. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * gateway: wire Confluence routes into Flask app (#1931) Phase 2 of the Confluence read-only gateway support. Adds the eight POST /api/v1/confluence/* routes alongside the existing /api/v1/jira/* block in gateway.py: - /page/get - /page/descendants - /page/footer-comments - /page/inline-comments - /space/list (allowlist-filtered) - /space/pages (spaceKey → spaceId resolution via list_spaces cache) - /search (CQL extractor + clamp) - /execute (allowlisted-path passthrough) Each route composes session-auth → private-mode → space-allowlist (post- fetch for page reads, pre-call for spaceKey-supplied routes) → client call → audit_log. Translates ConfluenceUpstreamForbidden to HTTP 403 with the dedicated `confluence_upstream_403` audit event. Translates ConfluenceResponseTooLarge to HTTP 413. Also extends `_reload_all_config()` to call `reload_confluence_*` and emit `confluence_config_reloaded`, mirroring the existing Jira reload. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * gateway: fix fail-open allowlist bypass on parent-fetch failure (#1931) reviewer_security NACK (cycle 1): both confluence_page_footer_comments and confluence_page_inline_comments swallowed parent-page fetch errors (ConfluenceCredentialsUnavailable / ConfluenceUpstreamError / ConfluenceUpstreamForbidden) into ``parent = None`` and the subsequent ``if parent is not None and ... != \"not_found\":`` block had no else branch, so a transient 5xx, upstream 403 (per-page restriction inheritance), or a not_found envelope from the page-level read would fall through to make_success and ship the comment body to the sandbox WITHOUT applying the space allowlist. Fix mirrors the existing fail-closed shape in confluence_page_descendants: add an explicit else branch that returns confluence_space_denied with reason="parent page space could not be resolved" when the parent fetch fails or the parent envelope is not_found. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * gateway: address reviewer_contract + reviewer_code_holistic NACKs (#1931) Cycle-2 NACK fixes: reviewer_contract findings: - (#3) Drop CONFLUENCE_SPACE_KEYS env-var line entirely from config/secrets.template.env per TASK-5-2 acceptance — replaced with a comment pointing operators at config/context-filters.yaml :: confluence.spaces (decision H1). - (#4) Invert BASE_URL precedence in confluence_credentials.py to match decision F1: ATLASSIAN_BASE_URL+/wiki wins when set, with CONFLUENCE_BASE_URL the per-key back-compat fallback only. Now consistent with the equivalent jira_credentials loader. - (TASK-5-3) Extend k8s/base/gateway-deployment.yaml comment block to enumerate the new ATLASSIAN_*/CONFLUENCE_* env keys for operator discoverability. Comment-only diff; kubectl apply --dry-run still succeeds. - (TASK-3-1) Stage sandbox/scripts/confluence content at .egg-state/agent-outputs/1931-sandbox-scripts-confluence so the coder role can push it through BRC despite the shared/egg_restrictions/patterns.py wholesale block on sandbox/scripts/. Added matching "sandbox/scripts/confluence" block_exempt_patterns entry so future re-proposes land directly (mirrors the resolution from #1556 sandbox/scripts/jira). Pre-merge obligation: maintainer must `git mv` the staged file to sandbox/scripts/confluence after the patterns.py exemption is live on the gateway pod. reviewer_code_holistic findings (3 blockers): - (#3) _resolve_space_key_via_list() now catches ConfluenceUpstreamForbidden alongside ConfluenceUpstreamError / ConfluenceCredentialsUnavailable. Forbidden on /wiki/api/v2/spaces (bot lacks space:read) is its own RuntimeError subclass — without this catch, Flask returns 500 instead of the documented 403/audit shape. Now fail-closes through confluence_space_denied. - (#1) Sandbox script staged (see TASK-3-1 above). - (#2) Comment-route fail-open already addressed in cycle-1 NACK follow-up commit c4d8fa0. Non-blocking improvements: - _log_default_body_format() now fires from get_page_footer_comments, get_page_inline_comments, and get_space_pages too — boot-time observability promise (decision-5 / risk R12) holds regardless of which Confluence verb is the first call. - redact_response() now also strips _links.self URLs that look like /api/vN/users/{accountId} (defense-in-depth against future Atlassian schema drift that drops accountId but keeps the link). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * docs: address cycle-2 doc gaps for Confluence wrapper (#1931) Documenter follow-up to coder commit 7744faf (cycle-2 NACK fixes). Aligns the four wrapper-related docs with the as-shipped code: - docs/reference/confluence-wrapper.md * Add _links.self redaction (defense-in-depth) to the response-redaction section; bump key count from "three" to "four". * Fix the BASE_URL precedence write-up: ATLASSIAN_BASE_URL+/wiki wins when set; CONFLUENCE_BASE_URL is the per-key back-compat fallback (matches confluence_credentials.py after cycle-2's inversion). * Make the comment-route fail-closed shape explicit — the parent-page fetch errors / not_found envelope route through confluence_space_denied (mirrors gateway.py fix in c4d8fa0). * Add a "Pre-merge obligation: sandbox script staging" section documenting the .egg-state/agent-outputs/ → sandbox/scripts/confluence git mv that the maintainer must run after merge. - docs/architecture/credential-injection.md * Same BASE_URL precedence fix (ATLASSIAN-wins) so the architecture doc matches the wrapper reference and the actual loader. - docs/architecture/network-isolation.md * Correct the test-coverage write-up: the substring assertion in test_allowed_domains.py catches wiki.atlassian.net / confluence.atlassian.com via the broader atlassian.net / atlassian.com parametrize entries; no per-Confluence row exists. - docs/index.md, sandbox/agent-config/rules/environment.md * Add user-profile _links.self to the redaction enumeration so the summary lines match the wrapper reference. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * sandbox: add Confluence CLI wrapper (#1931) Phase 3 — sandbox/scripts/confluence (Task 3-1): Bash CLI wrapper that routes Confluence commands through the gateway sidecar. Mirrors sandbox/scripts/jira shape: - Fail-closed on missing gateway sidecar. - EGG_SESSION_TOKEN Bearer auth on every call. - JSON-on-stdout, errors-on-stderr, non-zero exit on non-2xx. Verbs (Jira-style only per Q10): - page get / descendants / footer-comments / inline-comments - space pages / list - search '<CQL>' - execute <METHOD> <PATH> - help Each verb POSTs to the matching /api/v1/confluence/* endpoint where the gateway enforces space allowlist, CQL scope, response redaction, and the read-only fence. The wrapper itself never holds Atlassian credentials — gateway-side credential injection is the single trust boundary. Permitted via shared/egg_restrictions/patterns.py block_exempt_patterns landed in #2133/#2135 — the path 'sandbox/scripts/confluence' is the narrow exemption added alongside the existing 'sandbox/scripts/jira' exemption from #1556. * gateway: address tester NACK — ruff format + mypy clean (#1931) Tester NACK identified two blocking issues for ``make lint`` once the sandbox/scripts/confluence wrapper landed: 1. ``ruff format --check`` reported three coder-owned source files (gateway/confluence_client.py, gateway/confluence_credentials.py, gateway/gateway.py) needed reformatting — pure whitespace / line-break style, no logic change. Applied ``ruff format`` per the tester's diagnostic. 2. ``mypy`` then surfaced 12 latent type errors across the Confluence routes — ``data.get("pageId")`` returns ``Any | None``, but the ``ConfluenceClient`` methods are typed ``str``. After the existing ``_validate_confluence_page_id`` / ``_validate_confluence_space_key`` guard, the value is provably ``str``, but mypy can't follow ``tuple[bool, str]`` returns for narrowing. Added explicit ``assert isinstance(page_id|space_key, str)`` lines after each validator's reject-and-return block — single statement per route, no runtime cost beyond the assert, no behavioral change. 3. Reordered the import-ignore tags on the lazy-fallback Confluence imports in gateway.py: ``[no-redef, import-untyped]`` belongs on the FIRST import of each module (which is when mypy actually surfaces the missing-stubs error); subsequent imports of the same module only need ``[no-redef]``. Mypy was reporting the missing-stubs error at line 254 (first confluence_client import) and complaining that the same tag at line 260 was unused. Verification: - ``make lint`` passes cleanly (ruff check, ruff format check, mypy, shellcheck, custom checks). - ``pytest gateway/tests/test_jira_*.py`` all green (173 tests) — confirms the format reorderings on confluence_credentials.py didn't break the ATLASSIAN_*/JIRA_* per-key precedence path that test_jira_credentials.py exercises. Non-blocking findings from the tester (CQL function-name collision in ``_extract_space_clauses``, ``space_cache`` populates pre-allowlist-filter, ``CONFLUENCE_DENIED_VERBS`` mixes path segments and HTTP methods) are documented for follow-up but out of scope for this BRC round per the tester's own classification. * docs: address reviewer_code NACK on cycle-2 doc commit (#1931) Two reviewer_code blockers from NACK 2d5b7b98 on commit 8b46dfc: 1. Drop the "Pre-merge obligation: sandbox script staging" section from docs/reference/confluence-wrapper.md. Coder commit 72ee7dc committed sandbox/scripts/confluence at its canonical path; the shared/egg_restrictions/patterns.py block_exempt_patterns landed in #2133/#2135 already, so the staging route is obsolete and the git-mv instructions in the section were stale and would mislead maintainers. 2. Soften the "Anti-bypass invariant" paragraph in the /execute section. The previous wording claimed /execute rejects narrow- route path families (api/v2/pages/{id}, api/v2/spaces/{id}/pages, rest/api/search) and a regression test enforces this; neither is true at branch HEAD — gateway/confluence_client.py:163-175 ships those three patterns IN the /execute allowlist. Replaced with a "Known gap (tracked under issue #1931 cycle-2 NACK)" note that honestly describes the current state and points at the in-flight coder fix. Updated the Error cases table row and the Hard limits line in sandbox/agent-config/rules/environment.md to match. Non-blocking incorporated: spelled out the _USER_PROFILE_SELF_RE regex shape (/api/v\d+/users/) in the response-redaction bullet so future readers know v3+ users endpoints are also covered. Verified each change against gateway/confluence_client.py at the current branch HEAD (72ee7dc). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * gateway: address reviewer_code + reviewer_security NACK — drop /execute bypass paths (#1931) Cycle-3 NACKs from reviewer_code (9ae21669) and reviewer_security (ec5985ff) identified three concrete cross-partition bypasses in the /execute path allowlist that exactly match the pattern PR #1964 had to fix in the Jira allowlist (the ``^project$`` / ``search/jql`` exclusions in gateway/jira_client.py). All three were exploitable from inside a private-mode sandbox today and contradicted the explicit "Anti-bypass invariant" promised in docs/reference/confluence-wrapper.md and sandbox/agent-config/rules/environment.md. Blocking #1: ``rest/api/search`` in CONFLUENCE_API_ALLOWED_PATHS lets an agent POST {"method":"GET","path":"rest/api/search","query":{"cql":"text ~ \"secret\""}} through /execute and run arbitrary CQL — bypassing extract_search_spaces() entirely. Fix: drop ``re.compile(r"^rest/api/search$")`` from the allowlist. All CQL must now flow through /api/v1/confluence/search where the static extractor enforces space-scope. Blocking #2: ``api/v2/spaces`` in CONFLUENCE_API_ALLOWED_PATHS lets an agent enumerate the full tenant space catalog via execute_raw, which does NOT apply the allowlist filter that list_spaces does. Defeats decision-11 ("agents cannot enumerate the full tenant space set"). Fix: drop ``re.compile(r"^api/v2/spaces$")`` from the allowlist. Space enumeration must now flow through /api/v1/confluence/space/list which filters to the operator's allowlist. Blocking #3: ``api/v2/footer-comments`` and ``api/v2/inline-comments`` (the flat v2 endpoints) accept ``page-id`` in the query string, but no ``spaceKey`` filter exists at upstream. An agent passes {"path":"api/v2/footer-comments","query":{"spaceKey":"ALLOWED", "page-id":"<NON_ALLOWLISTED_PAGE>"}} — the up-front spaceKey gate sees ``ALLOWED``, Atlassian ignores ``spaceKey`` and returns comments from the non-allowlisted page. The audit log records ``spaceKey=ALLOWED``, masking the exfil. Fix: drop both ``re.compile(r"^api/v2/footer-comments$")`` and ``re.compile(r"^api/v2/inline-comments$")`` from the allowlist. The narrow /api/v1/confluence/page/{footer,inline}-comments routes already cover the agent-facing use case AND correctly fetch the parent page to verify its space (with the cycle-1 fail-closed fix on parent-fetch failure from c4d8fa0). Important: these four paths remain reachable INTERNALLY by ConfluenceClient methods that construct them directly (the include_replies side-call inside get_page_footer_comments and the v2-bug fallback inside get_page_inline_comments) — those paths do NOT go through validate_confluence_api_path. Only the agent-facing /execute escape hatch is closed. Stale-artifact cleanup (reviewer_code_holistic 183e44a0 partial fix): delete the now-redundant staged copy at .egg-state/agent-outputs/1931-sandbox-scripts-confluence. Cycle-3 commit 72ee7dc already pushed the wrapper to its real on-disk path (sandbox/scripts/confluence), making the staging artifact dead weight. gateway/gateway.py /execute route: strip the dead ``requires_space_key`` branch that special-cased the four removed paths. Reaching the post-validation block now implies a page- or space-scoped path family, all of which carry an id inline that the existing post-fetch allowlist check resolves to a spaceKey. Verification: - ``make lint`` passes (ruff check + ruff format check + mypy + shellcheck + custom checks). - ``pytest gateway/tests/test_jira_*.py`` all green (173 tests) — Jira reads / search / execute paths unchanged. Out of scope for this commit (handed back to reviewer_code_holistic): - Aligning patterns.py with main #2135 (drop wholesale ``sandbox/scripts/`` block + drop both ``sandbox/scripts/jira`` and ``sandbox/scripts/confluence`` exemptions) requires a coordinated tester-scope test update on the same branch (the gh / git-credential-github-token test assertions in gateway/tests/test_agent_restrictions_*.py and shared/tests/test_egg_restrictions.py expect the wholesale block to remain — they all flip in main #2135). Those test files are blocked for the coder role (tester scope), so the patterns.py alignment cannot be done unilaterally without breaking the test suite on this branch. Recommend resolving via main merge at PR time, or via a dedicated tester-coder coordinated cycle. - The non-blocking cosmetic / observability findings from the three reviewer NACKs (CQL function-name collision, audit-log gap on 413, redundant ``self.space_cache.put`` skip in get_page, eager spaceId cache population, log-once flag scoping, Jira-loader log spam, heredoc quote on gateway-down message, %-encoded smuggling decoder) are tracked for follow-up tickets — every adversarial probe in the test grid is rejected today. * tests: add Confluence gateway test suite (#1931) Adds 245+ tests covering Phase 4 acceptance criteria for the Confluence gateway wrapper introduced in #1931: - gateway/tests/test_confluence_credentials.py — F1 ATLASSIAN_*/CONFLUENCE_* per-key precedence matrix, /wiki suffix derivation, mtime cache + reload. - gateway/tests/test_confluence_policy.py — context-filters.yaml round-trip, fail-closed semantics, mixed-case key preservation, mtime + manual reload. - gateway/tests/test_confluence_search.py — CQL space-scope extractor: positive shapes (space = K, space IN (K, ...)) plus 17 adversarial cases (OR, capitalisation, quoted keys, CQL functions, comments, semicolons, unicode homoglyphs, bare id/title/content clauses, missing scope). - gateway/tests/test_confluence_client.py — httpx.MockTransport coverage of every public verb, validate_confluence_api_path positive + negative grids, 429 single-retry with Retry-After clamp at 30s, 404 envelope vs raise semantics, 403 → ConfluenceUpstreamForbidden, v1 inline-comment fallback, footer-comment nested-reply merge, list_spaces case-sensitive allowlist filter, redact_response (incl. ADF mention nodes), payload-size cap. - gateway/tests/test_confluence_routes.py — eight POST routes end-to-end: public-mode 403 + private_mode_required audit, route-enumeration regression (every view carries __egg_requires_private_mode__), disallowed-space body-leak guard, route-vs-execute anti-bypass for /execute, adversarial CQL through the route, used_fallback observability, page/descendants risk-R8 default depth=1/limit=25, audit-shape regression. - tests/sandbox/test_confluence_wrapper.py — subprocess-driven tests for the bash wrapper: per-verb request body shape, Authorization header, exit-code contract, fail-closed on missing token / unreachable gateway. - gateway/tests/test_jira_credentials.py — extends the existing suite with the ATLASSIAN_* precedence matrix called for in plan task 4-1b / risk R11. - gateway/tests/test_allowed_domains.py — extends parametrize list with wiki.atlassian.net and confluence.atlassian.com defensive entries. - gateway/tests/conftest.py — loads confluence_{credentials,client,policy, search} modules so the route-tests see the same Flask app the production loader builds. All tests pass via `pytest gateway/tests/test_confluence_*.py gateway/tests/test_jira_credentials.py gateway/tests/test_allowed_domains.py tests/sandbox/test_confluence_wrapper.py` (245 tests, 0 failures). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * docs: restore strict /execute anti-bypass invariant after cycle-3 fix (#1931) Cycle-3 commit f3f552e closed the four flat-v2 bypass paths in gateway/confluence_client.py::CONFLUENCE_API_ALLOWED_PATHS: - rest/api/search (CQL extractor bypass) - api/v2/spaces (allowlist-filter bypass) - api/v2/footer-comments (page-id-in-query, no upstream spaceKey filter) - api/v2/inline-comments (same flat-endpoint shape) The cycle-3 work: - Restores the strict "Anti-bypass invariant" section (replacing the prior cycle-2 "Known gap" placeholder) in docs/reference/confluence-wrapper.md, with a concrete table of the four removed paths + the bypass each would have enabled, and a pointer at the regression tests pinning the rejection (gateway/tests/test_confluence_client.py + test_confluence_routes.py end-to-end via Flask test client) that the tester landed in 6b44b59. - Updates the "Allowed path families" bullet in the /execute section to list only the six remaining path families (all page- or space-scoped, all carry an inline id) and explicitly call out the four exclusions. - Updates the Error cases table row to drop the "Known gap" pointer and describe the disallowed-path-family reason concretely. - Updates the Hard limits line in sandbox/agent-config/rules/environment.md to enumerate the four refused paths and direct callers to the narrow verbs (confluence search, confluence space list, confluence page footer-comments, confluence page inline-comments). Verified each enumeration against gateway/confluence_client.py:183-192 at branch HEAD (rebased onto 6b44b59). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * Address PR #2141 review: pagination, redaction, lock, allowlist Six fixes from review feedback on PR #2141: 1. populate_space_cache(): new method that walks _links.next pagination (capped at 4 pages) so /space/list cache lookups for spaceKey translation see all allowlisted spaces, not just the first 25. Preserves cursor semantics for the user-facing /space/list route by keeping list_spaces single-page. 2. Redact upstream error body: ConfluenceUpstreamError 5xx bodies are passed through redact_response before being included in the confluence_upstream_error response, so accountId/emailAddress leaked by Atlassian errors do not reach the sandbox. 3. Lock around lazy _client(): double-checked locking on ConfluenceClient.http_client construction to avoid two concurrent first-callers each creating a separate httpx.Client. 4. Drop descendants / comments / v1-comment paths from /execute allowlist: the dedicated narrow routes (/page/descendants, /page/footer-comments, /page/inline-comments) already enforce policy for those reads; /execute should not duplicate them. The remaining allowlist is just `api/v2/pages/{id}` and `api/v2/spaces/{id}/pages`. 5. Tighten CQL extractor rejection message for id/content/title clauses: now "id, content, and title clauses are not supported; use 'text ~ ...' instead" — points agents at the supported shape. 6. Rename _contains_top_level_or → _contains_or and _contains_bare_id_clause → _contains_id_clause to match the actual semantics (the regex matches at any depth, not just the top level; id/content/title are rejected with or without a space anchor). Tests: 24 new / updated assertions across test_confluence_client.py, test_confluence_routes.py, and test_confluence_search.py covering pagination, error-body redaction, lazy-client lock, and the new rejection-reason wording. Docs updated to reflect the tightened /execute allowlist. — Authored by egg * Address PR #2141 review observations: cursor comment + execute cache-miss test - Add inline comment to _extract_next_cursor explaining parse_qs blank-cursor fail-safe (observation #4 from egg-reviewer 997578d review). - Add direct route-level test for confluence_execute warming the paginated space cache via the api/v2/spaces/<id>/pages branch (observation #3 — the branch was previously only exercised indirectly through /space/pages). Author: egg <egg@localhost> * Catch ConfluenceUpstreamForbidden in confluence_execute cache warm Closes the blocking finding from the latest review on PR #2141: when populate_space_cache raises ConfluenceUpstreamForbidden during the space_id_in_path cache-miss branch (bot lacks space:read globally), the exception escaped as a Flask 500 instead of fail-closing through confluence_space_denied. ConfluenceUpstreamForbidden is a sibling of ConfluenceUpstreamError — both inherit directly from RuntimeError, not one from the other — so the existing exception tuple did not catch it. Mirrors the handler at _resolve_space_key_via_list. Also addresses two non-blocking observations: - Reword the _extract_next_cursor inline comment so it acknowledges the existing ``cursor or None`` guard rather than implying it would need to be added. - Extend the cache-miss test matrix for the space_id_in_path branch: - populate_space_cache raises ConfluenceUpstreamForbidden -> 403 - populate_space_cache succeeds but id stays unresolved -> 403 - resolved key is not in the operator allowlist -> 403 + audited key All three regression tests assert the upstream payload is not leaked to the agent on denial. Authored-by: egg * Reflect per-call-site 403 audit translation in docstring The ConfluenceUpstreamForbidden docstring claimed the route layer uniformly emits confluence_upstream_403 audit events, but in reality only confluence_space_pages does. _resolve_space_key_via_list (via confluence_search) and confluence_execute collapse the 403 into the route's *_denied event so allowlist resolution stays fail-closed and does not expose tenant-permission state. Updates the docstring to describe each call site's actual behaviour (reviewer follow-up option 2 from PR #2141 re-review). * Fix per-call-site 403 docstring to match all nine routes Reviewer flagged three accuracy issues in the ConfluenceUpstreamForbidden docstring on a7778f2: 1. confluence_search catches ConfluenceUpstreamForbidden directly and translates to confluence_upstream_403; the docstring incorrectly attributed it to _resolve_space_key_via_list. 2. Eight routes (not just confluence_space_pages) translate the 403 directly. The actual asymmetry is primary route call vs. auxiliary allowlist-resolution call. 3. The module-level docstring still claimed uniform translation, contradicting the new class docstring. Class docstring now enumerates all eight primary routes and the three auxiliary call sites that collapse the 403 (resolve_space_key_via_list, the parent re-fetch in descendants/footer-comments/inline-comments, and the execute cache-warm fallback). Module-level docstring redirects to the class docstring instead of repeating the inaccurate "all read methods" framing. --------- Co-authored-by: egg-orchestrator <egg@localhost> Co-authored-by: egg <egg@example.com> Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com> Co-authored-by: jwbron <8340608+jwbron@users.noreply.github.com> Co-authored-by: egg-reviewer[bot] <261018737+egg-reviewer[bot]@users.noreply.github.com> Co-authored-by: james-in-a-box[bot] <246424927+james-in-a-box[bot]@users.noreply.github.com>

The fallback in `tests/sandbox/test_jira_wrapper.py` was scaffolding for the pre-#2135 workaround when `sandbox/scripts/` was blocked from agent writes. With #2135 merged, new wrappers land at the canonical path directly and the artifact location is permanently dead code. Drops the docstring paragraph, the `_ARTIFACT` constant, and the fallback branch in `_locate_wrapper()`; simplifies the skip message.

…nly) (#2141) * Initialize SDLC contract for issue #1931 * refine: analysis for #1931 (Confluence gateway read-only v1) Drafts the refine-phase analysis for issue #1931 — Confluence gateway read-only support, mirroring the Jira gateway pattern from #1556. Captures the Confluence v1/v2 API split, the v2-first hybrid (with v1 CQL search and v1 fallbacks for known v2 comment bugs), the space allowlist + verb allowlist, the private-mode-only restriction, and the shared Atlassian credential strategy with the Jira gateway. All fourteen multiple-choice decisions and ten free-form feedback items are registered against the contract. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * Persist statefiles after refine phase * Persist HITL resolution after refine phase gate * plan(architect): add architecture analysis for #1931 Confluence gateway Mirrors the Jira gateway pattern from #1556 with Confluence-specific adaptations (v2-first / v1-fallback hybrid, CQL static-scope extractor, shared ATLASSIAN_* credential triple). Records all 14 HITL decision resolutions and 10 free-form feedback answers from refine. Includes component breakdown, route surface, data-flow walkthrough, and 4 plan- phase open questions for the task_planner / risk_analyst. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * plan: risk assessment for #1931 (Confluence gateway read-only v1) Plan-phase risk_analyst output: 18 risks across security (6), performance (3), compatibility (2), operational (3), data-privacy (1), external-dependency (1), test-coverage (1), and future-write (1). Severity distribution: 1 high (CQL extractor adversarial coverage), 7 medium (most ride on the #1556 Jira-gateway scaffolding so the implementation surface is well-understood), 10 low. Three risks flagged for human review at implement time: - R1: CQL-extractor parity with the JQL adversarial suite must target CQL-specific grammar (text ~ contains, space.category(), etc.). - R14: attachments / restrictions / permissions permanent-denylist enforcement across both narrow-route and /execute paths. - R15: bot-account effective-access asymmetry (per feedback Q9) — must surface a structured forbidden envelope so agents do not retry. External research covered Atlassian's April 2026 v1 deprecation status (endpoint-specific, CQL search has no v2 successor), the March 2026 points-based rate-limit rollout, and confirmed no public CQL-injection CVE for 2025-2026. Includes a four-level rollback plan (config-only -> route-disable -> credential-revoke -> full-revert) and an 18-item implement-phase checklist that maps each risk to a concrete reviewer-verifiable mitigation. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * plan: add task plan for #1931 (Confluence gateway read-only v1) Decomposes the architecture analysis into a single-PR plan with 6 phases and 28 tasks, mirroring the #1556 Jira-gateway scaffolding. Incorporates all 14 refine HITL decisions (verb-noun URL paths, v2-first hybrid, conservative CQL extractor, body-format=storage default per the operator tweak, shared ATLASSIAN_* triple, per-route private-mode gate, context-filters.yaml allowlist, attachments denylist, no per-pipeline env vars, GET-only /execute) plus the architect's per-verb endpoint pinning, page->space resolution caching, and the risk analyst's R1-R18 mitigations (5 MiB payload cap, descendants depth=1/limit=25 default, bot_account_lacks_read_access reason on 403, confluence_v1_fallback audit, ADF redaction, attachments-denylist case/encoding/nesting tests, route-vs-execute anti-bypass test, prompt-injection caveat in docs). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * plan: address reviewer_plan NACK (#1931) Blocking fixes: - Add TASK-1-5 to update gateway/jira_credentials.py for the same ATLASSIAN_*-preferred / JIRA_* fall-back precedence (risk R11). Add TASK-4-1b extending test_jira_credentials.py with the six-combination matrix. Update PR description and pre-merge note so the shared-credential migration story is portable (operators can drop the legacy JIRA_* block once ATLASSIAN_* is fully populated without silently breaking Jira). - Specify v1-also-404 fall-through behaviour for inline-comment fallback in TASK-1-2: v1 200-empty -> {results: [], used_fallback}; v1 404 -> standard not_found envelope with used_fallback=true. Non-blocking fixes: - Replace "decision E5" with "decision-5" (8 sites). - Move boot-time policy log to confluence_policy.py; client logs only body-format default; credentials log precedence (risk R12). - Add `attempt: 1|2` to confluence_upstream_rate_limited audit shape. - Two-sided spaceId<->spaceKey LRU cache populated by both list_spaces and get_page so /space/pages cold-start avoids double round-trip. - Mention architect Q4 in TASK-1-1 acceptance. - Replace PRIVATE_MODE env-var references with private-mode session language (private mode is g.session_mode, not a process env var). - Note TASK-4-7 must verify existing test name at implement time. - Note show-metrics.md is intentionally untouched. - Add Atlassian rate-limit pool-sharing note to TASK-6-4. - TASK-2-9 audit-event shape mirrors whatever the existing Jira reload emits. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * Persist statefiles after plan phase * docs(implement): add Confluence gateway wrapper docs (#1931) Phase 6 of the read-only Confluence gateway wrapper. New docs/reference/confluence-wrapper.md mirrors the Jira reference and covers the eight `/api/v1/confluence/*` routes, the conservative CQL extractor, the `not_found` envelope, the v1 inline-comment fallback, the response-redaction walker (accountId / emailAddress / user-profile _links.webui), the bot-vs-human access caveat, the prompt-injection caveat, the Atlassian rate-limit runbook (with shared-bot quota-pool note), and the future write-verb extension points. Architecture docs gain a Confluence row in the gateway endpoint table, a Confluence section in credential injection covering the shared `ATLASSIAN_*` precedence and `/wiki` base-URL derivation, and an extended Squid allowlist exclusion paragraph naming Confluence hostnames. The sandbox environment rules document the new `confluence` wrapper verbs and call out that no per-pipeline env var is exported (Confluence is reference material, not a unit of work). docs/index.md adds the wrapper to the reference lookup table. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * gateway: Confluence foundation modules + ATLASSIAN_* credential precedence (#1931) Phase 1 of the Confluence read-only gateway support. Introduces the Confluence-specific building blocks the routes will compose: - gateway/confluence_credentials.py — ATLASSIAN_*/CONFLUENCE_* per-key precedence with /wiki base derivation, mtime-cached, thread-safe. - gateway/confluence_client.py — class-shaped v2-first client with v1 fallbacks for inline-comment 404 and footer-comment nested replies, redaction (accountId / emailAddress / user-profile webui links), payload-size cap (5 MiB), 429 single-retry, 403 envelope, space cache. - gateway/confluence_policy.py — confluence.spaces YAML allowlist loader with mtime cache and fail-closed semantics. - gateway/confluence_search.py — conservative CQL space=/space IN(...) extractor mirroring jira_search's deny-on-ambiguity stance. - gateway/jira_credentials.py — extended to honor ATLASSIAN_*/JIRA_* per-key precedence so the shared-credential migration is portable. Also adds confluence: section to config/context-filters.yaml (empty, fail-closed) and ATLASSIAN_* triple to config/secrets.template.env. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * gateway: wire Confluence routes into Flask app (#1931) Phase 2 of the Confluence read-only gateway support. Adds the eight POST /api/v1/confluence/* routes alongside the existing /api/v1/jira/* block in gateway.py: - /page/get - /page/descendants - /page/footer-comments - /page/inline-comments - /space/list (allowlist-filtered) - /space/pages (spaceKey → spaceId resolution via list_spaces cache) - /search (CQL extractor + clamp) - /execute (allowlisted-path passthrough) Each route composes session-auth → private-mode → space-allowlist (post- fetch for page reads, pre-call for spaceKey-supplied routes) → client call → audit_log. Translates ConfluenceUpstreamForbidden to HTTP 403 with the dedicated `confluence_upstream_403` audit event. Translates ConfluenceResponseTooLarge to HTTP 413. Also extends `_reload_all_config()` to call `reload_confluence_*` and emit `confluence_config_reloaded`, mirroring the existing Jira reload. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * gateway: fix fail-open allowlist bypass on parent-fetch failure (#1931) reviewer_security NACK (cycle 1): both confluence_page_footer_comments and confluence_page_inline_comments swallowed parent-page fetch errors (ConfluenceCredentialsUnavailable / ConfluenceUpstreamError / ConfluenceUpstreamForbidden) into ``parent = None`` and the subsequent ``if parent is not None and ... != \"not_found\":`` block had no else branch, so a transient 5xx, upstream 403 (per-page restriction inheritance), or a not_found envelope from the page-level read would fall through to make_success and ship the comment body to the sandbox WITHOUT applying the space allowlist. Fix mirrors the existing fail-closed shape in confluence_page_descendants: add an explicit else branch that returns confluence_space_denied with reason="parent page space could not be resolved" when the parent fetch fails or the parent envelope is not_found. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * gateway: address reviewer_contract + reviewer_code_holistic NACKs (#1931) Cycle-2 NACK fixes: reviewer_contract findings: - (#3) Drop CONFLUENCE_SPACE_KEYS env-var line entirely from config/secrets.template.env per TASK-5-2 acceptance — replaced with a comment pointing operators at config/context-filters.yaml :: confluence.spaces (decision H1). - (#4) Invert BASE_URL precedence in confluence_credentials.py to match decision F1: ATLASSIAN_BASE_URL+/wiki wins when set, with CONFLUENCE_BASE_URL the per-key back-compat fallback only. Now consistent with the equivalent jira_credentials loader. - (TASK-5-3) Extend k8s/base/gateway-deployment.yaml comment block to enumerate the new ATLASSIAN_*/CONFLUENCE_* env keys for operator discoverability. Comment-only diff; kubectl apply --dry-run still succeeds. - (TASK-3-1) Stage sandbox/scripts/confluence content at .egg-state/agent-outputs/1931-sandbox-scripts-confluence so the coder role can push it through BRC despite the shared/egg_restrictions/patterns.py wholesale block on sandbox/scripts/. Added matching "sandbox/scripts/confluence" block_exempt_patterns entry so future re-proposes land directly (mirrors the resolution from #1556 sandbox/scripts/jira). Pre-merge obligation: maintainer must `git mv` the staged file to sandbox/scripts/confluence after the patterns.py exemption is live on the gateway pod. reviewer_code_holistic findings (3 blockers): - (#3) _resolve_space_key_via_list() now catches ConfluenceUpstreamForbidden alongside ConfluenceUpstreamError / ConfluenceCredentialsUnavailable. Forbidden on /wiki/api/v2/spaces (bot lacks space:read) is its own RuntimeError subclass — without this catch, Flask returns 500 instead of the documented 403/audit shape. Now fail-closes through confluence_space_denied. - (#1) Sandbox script staged (see TASK-3-1 above). - (#2) Comment-route fail-open already addressed in cycle-1 NACK follow-up commit c4d8fa0. Non-blocking improvements: - _log_default_body_format() now fires from get_page_footer_comments, get_page_inline_comments, and get_space_pages too — boot-time observability promise (decision-5 / risk R12) holds regardless of which Confluence verb is the first call. - redact_response() now also strips _links.self URLs that look like /api/vN/users/{accountId} (defense-in-depth against future Atlassian schema drift that drops accountId but keeps the link). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * docs: address cycle-2 doc gaps for Confluence wrapper (#1931) Documenter follow-up to coder commit 7744faf (cycle-2 NACK fixes). Aligns the four wrapper-related docs with the as-shipped code: - docs/reference/confluence-wrapper.md * Add _links.self redaction (defense-in-depth) to the response-redaction section; bump key count from "three" to "four". * Fix the BASE_URL precedence write-up: ATLASSIAN_BASE_URL+/wiki wins when set; CONFLUENCE_BASE_URL is the per-key back-compat fallback (matches confluence_credentials.py after cycle-2's inversion). * Make the comment-route fail-closed shape explicit — the parent-page fetch errors / not_found envelope route through confluence_space_denied (mirrors gateway.py fix in c4d8fa0). * Add a "Pre-merge obligation: sandbox script staging" section documenting the .egg-state/agent-outputs/ → sandbox/scripts/confluence git mv that the maintainer must run after merge. - docs/architecture/credential-injection.md * Same BASE_URL precedence fix (ATLASSIAN-wins) so the architecture doc matches the wrapper reference and the actual loader. - docs/architecture/network-isolation.md * Correct the test-coverage write-up: the substring assertion in test_allowed_domains.py catches wiki.atlassian.net / confluence.atlassian.com via the broader atlassian.net / atlassian.com parametrize entries; no per-Confluence row exists. - docs/index.md, sandbox/agent-config/rules/environment.md * Add user-profile _links.self to the redaction enumeration so the summary lines match the wrapper reference. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * sandbox: add Confluence CLI wrapper (#1931) Phase 3 — sandbox/scripts/confluence (Task 3-1): Bash CLI wrapper that routes Confluence commands through the gateway sidecar. Mirrors sandbox/scripts/jira shape: - Fail-closed on missing gateway sidecar. - EGG_SESSION_TOKEN Bearer auth on every call. - JSON-on-stdout, errors-on-stderr, non-zero exit on non-2xx. Verbs (Jira-style only per Q10): - page get / descendants / footer-comments / inline-comments - space pages / list - search '<CQL>' - execute <METHOD> <PATH> - help Each verb POSTs to the matching /api/v1/confluence/* endpoint where the gateway enforces space allowlist, CQL scope, response redaction, and the read-only fence. The wrapper itself never holds Atlassian credentials — gateway-side credential injection is the single trust boundary. Permitted via shared/egg_restrictions/patterns.py block_exempt_patterns landed in #2133/#2135 — the path 'sandbox/scripts/confluence' is the narrow exemption added alongside the existing 'sandbox/scripts/jira' exemption from #1556. * gateway: address tester NACK — ruff format + mypy clean (#1931) Tester NACK identified two blocking issues for ``make lint`` once the sandbox/scripts/confluence wrapper landed: 1. ``ruff format --check`` reported three coder-owned source files (gateway/confluence_client.py, gateway/confluence_credentials.py, gateway/gateway.py) needed reformatting — pure whitespace / line-break style, no logic change. Applied ``ruff format`` per the tester's diagnostic. 2. ``mypy`` then surfaced 12 latent type errors across the Confluence routes — ``data.get("pageId")`` returns ``Any | None``, but the ``ConfluenceClient`` methods are typed ``str``. After the existing ``_validate_confluence_page_id`` / ``_validate_confluence_space_key`` guard, the value is provably ``str``, but mypy can't follow ``tuple[bool, str]`` returns for narrowing. Added explicit ``assert isinstance(page_id|space_key, str)`` lines after each validator's reject-and-return block — single statement per route, no runtime cost beyond the assert, no behavioral change. 3. Reordered the import-ignore tags on the lazy-fallback Confluence imports in gateway.py: ``[no-redef, import-untyped]`` belongs on the FIRST import of each module (which is when mypy actually surfaces the missing-stubs error); subsequent imports of the same module only need ``[no-redef]``. Mypy was reporting the missing-stubs error at line 254 (first confluence_client import) and complaining that the same tag at line 260 was unused. Verification: - ``make lint`` passes cleanly (ruff check, ruff format check, mypy, shellcheck, custom checks). - ``pytest gateway/tests/test_jira_*.py`` all green (173 tests) — confirms the format reorderings on confluence_credentials.py didn't break the ATLASSIAN_*/JIRA_* per-key precedence path that test_jira_credentials.py exercises. Non-blocking findings from the tester (CQL function-name collision in ``_extract_space_clauses``, ``space_cache`` populates pre-allowlist-filter, ``CONFLUENCE_DENIED_VERBS`` mixes path segments and HTTP methods) are documented for follow-up but out of scope for this BRC round per the tester's own classification. * docs: address reviewer_code NACK on cycle-2 doc commit (#1931) Two reviewer_code blockers from NACK 2d5b7b98 on commit 8b46dfc: 1. Drop the "Pre-merge obligation: sandbox script staging" section from docs/reference/confluence-wrapper.md. Coder commit 72ee7dc committed sandbox/scripts/confluence at its canonical path; the shared/egg_restrictions/patterns.py block_exempt_patterns landed in #2133/#2135 already, so the staging route is obsolete and the git-mv instructions in the section were stale and would mislead maintainers. 2. Soften the "Anti-bypass invariant" paragraph in the /execute section. The previous wording claimed /execute rejects narrow- route path families (api/v2/pages/{id}, api/v2/spaces/{id}/pages, rest/api/search) and a regression test enforces this; neither is true at branch HEAD — gateway/confluence_client.py:163-175 ships those three patterns IN the /execute allowlist. Replaced with a "Known gap (tracked under issue #1931 cycle-2 NACK)" note that honestly describes the current state and points at the in-flight coder fix. Updated the Error cases table row and the Hard limits line in sandbox/agent-config/rules/environment.md to match. Non-blocking incorporated: spelled out the _USER_PROFILE_SELF_RE regex shape (/api/v\d+/users/) in the response-redaction bullet so future readers know v3+ users endpoints are also covered. Verified each change against gateway/confluence_client.py at the current branch HEAD (72ee7dc). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * gateway: address reviewer_code + reviewer_security NACK — drop /execute bypass paths (#1931) Cycle-3 NACKs from reviewer_code (9ae21669) and reviewer_security (ec5985ff) identified three concrete cross-partition bypasses in the /execute path allowlist that exactly match the pattern PR #1964 had to fix in the Jira allowlist (the ``^project$`` / ``search/jql`` exclusions in gateway/jira_client.py). All three were exploitable from inside a private-mode sandbox today and contradicted the explicit "Anti-bypass invariant" promised in docs/reference/confluence-wrapper.md and sandbox/agent-config/rules/environment.md. Blocking #1: ``rest/api/search`` in CONFLUENCE_API_ALLOWED_PATHS lets an agent POST {"method":"GET","path":"rest/api/search","query":{"cql":"text ~ \"secret\""}} through /execute and run arbitrary CQL — bypassing extract_search_spaces() entirely. Fix: drop ``re.compile(r"^rest/api/search$")`` from the allowlist. All CQL must now flow through /api/v1/confluence/search where the static extractor enforces space-scope. Blocking #2: ``api/v2/spaces`` in CONFLUENCE_API_ALLOWED_PATHS lets an agent enumerate the full tenant space catalog via execute_raw, which does NOT apply the allowlist filter that list_spaces does. Defeats decision-11 ("agents cannot enumerate the full tenant space set"). Fix: drop ``re.compile(r"^api/v2/spaces$")`` from the allowlist. Space enumeration must now flow through /api/v1/confluence/space/list which filters to the operator's allowlist. Blocking #3: ``api/v2/footer-comments`` and ``api/v2/inline-comments`` (the flat v2 endpoints) accept ``page-id`` in the query string, but no ``spaceKey`` filter exists at upstream. An agent passes {"path":"api/v2/footer-comments","query":{"spaceKey":"ALLOWED", "page-id":"<NON_ALLOWLISTED_PAGE>"}} — the up-front spaceKey gate sees ``ALLOWED``, Atlassian ignores ``spaceKey`` and returns comments from the non-allowlisted page. The audit log records ``spaceKey=ALLOWED``, masking the exfil. Fix: drop both ``re.compile(r"^api/v2/footer-comments$")`` and ``re.compile(r"^api/v2/inline-comments$")`` from the allowlist. The narrow /api/v1/confluence/page/{footer,inline}-comments routes already cover the agent-facing use case AND correctly fetch the parent page to verify its space (with the cycle-1 fail-closed fix on parent-fetch failure from c4d8fa0). Important: these four paths remain reachable INTERNALLY by ConfluenceClient methods that construct them directly (the include_replies side-call inside get_page_footer_comments and the v2-bug fallback inside get_page_inline_comments) — those paths do NOT go through validate_confluence_api_path. Only the agent-facing /execute escape hatch is closed. Stale-artifact cleanup (reviewer_code_holistic 183e44a0 partial fix): delete the now-redundant staged copy at .egg-state/agent-outputs/1931-sandbox-scripts-confluence. Cycle-3 commit 72ee7dc already pushed the wrapper to its real on-disk path (sandbox/scripts/confluence), making the staging artifact dead weight. gateway/gateway.py /execute route: strip the dead ``requires_space_key`` branch that special-cased the four removed paths. Reaching the post-validation block now implies a page- or space-scoped path family, all of which carry an id inline that the existing post-fetch allowlist check resolves to a spaceKey. Verification: - ``make lint`` passes (ruff check + ruff format check + mypy + shellcheck + custom checks). - ``pytest gateway/tests/test_jira_*.py`` all green (173 tests) — Jira reads / search / execute paths unchanged. Out of scope for this commit (handed back to reviewer_code_holistic): - Aligning patterns.py with main #2135 (drop wholesale ``sandbox/scripts/`` block + drop both ``sandbox/scripts/jira`` and ``sandbox/scripts/confluence`` exemptions) requires a coordinated tester-scope test update on the same branch (the gh / git-credential-github-token test assertions in gateway/tests/test_agent_restrictions_*.py and shared/tests/test_egg_restrictions.py expect the wholesale block to remain — they all flip in main #2135). Those test files are blocked for the coder role (tester scope), so the patterns.py alignment cannot be done unilaterally without breaking the test suite on this branch. Recommend resolving via main merge at PR time, or via a dedicated tester-coder coordinated cycle. - The non-blocking cosmetic / observability findings from the three reviewer NACKs (CQL function-name collision, audit-log gap on 413, redundant ``self.space_cache.put`` skip in get_page, eager spaceId cache population, log-once flag scoping, Jira-loader log spam, heredoc quote on gateway-down message, %-encoded smuggling decoder) are tracked for follow-up tickets — every adversarial probe in the test grid is rejected today. * tests: add Confluence gateway test suite (#1931) Adds 245+ tests covering Phase 4 acceptance criteria for the Confluence gateway wrapper introduced in #1931: - gateway/tests/test_confluence_credentials.py — F1 ATLASSIAN_*/CONFLUENCE_* per-key precedence matrix, /wiki suffix derivation, mtime cache + reload. - gateway/tests/test_confluence_policy.py — context-filters.yaml round-trip, fail-closed semantics, mixed-case key preservation, mtime + manual reload. - gateway/tests/test_confluence_search.py — CQL space-scope extractor: positive shapes (space = K, space IN (K, ...)) plus 17 adversarial cases (OR, capitalisation, quoted keys, CQL functions, comments, semicolons, unicode homoglyphs, bare id/title/content clauses, missing scope). - gateway/tests/test_confluence_client.py — httpx.MockTransport coverage of every public verb, validate_confluence_api_path positive + negative grids, 429 single-retry with Retry-After clamp at 30s, 404 envelope vs raise semantics, 403 → ConfluenceUpstreamForbidden, v1 inline-comment fallback, footer-comment nested-reply merge, list_spaces case-sensitive allowlist filter, redact_response (incl. ADF mention nodes), payload-size cap. - gateway/tests/test_confluence_routes.py — eight POST routes end-to-end: public-mode 403 + private_mode_required audit, route-enumeration regression (every view carries __egg_requires_private_mode__), disallowed-space body-leak guard, route-vs-execute anti-bypass for /execute, adversarial CQL through the route, used_fallback observability, page/descendants risk-R8 default depth=1/limit=25, audit-shape regression. - tests/sandbox/test_confluence_wrapper.py — subprocess-driven tests for the bash wrapper: per-verb request body shape, Authorization header, exit-code contract, fail-closed on missing token / unreachable gateway. - gateway/tests/test_jira_credentials.py — extends the existing suite with the ATLASSIAN_* precedence matrix called for in plan task 4-1b / risk R11. - gateway/tests/test_allowed_domains.py — extends parametrize list with wiki.atlassian.net and confluence.atlassian.com defensive entries. - gateway/tests/conftest.py — loads confluence_{credentials,client,policy, search} modules so the route-tests see the same Flask app the production loader builds. All tests pass via `pytest gateway/tests/test_confluence_*.py gateway/tests/test_jira_credentials.py gateway/tests/test_allowed_domains.py tests/sandbox/test_confluence_wrapper.py` (245 tests, 0 failures). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * docs: restore strict /execute anti-bypass invariant after cycle-3 fix (#1931) Cycle-3 commit f3f552e closed the four flat-v2 bypass paths in gateway/confluence_client.py::CONFLUENCE_API_ALLOWED_PATHS: - rest/api/search (CQL extractor bypass) - api/v2/spaces (allowlist-filter bypass) - api/v2/footer-comments (page-id-in-query, no upstream spaceKey filter) - api/v2/inline-comments (same flat-endpoint shape) The cycle-3 work: - Restores the strict "Anti-bypass invariant" section (replacing the prior cycle-2 "Known gap" placeholder) in docs/reference/confluence-wrapper.md, with a concrete table of the four removed paths + the bypass each would have enabled, and a pointer at the regression tests pinning the rejection (gateway/tests/test_confluence_client.py + test_confluence_routes.py end-to-end via Flask test client) that the tester landed in 6b44b59. - Updates the "Allowed path families" bullet in the /execute section to list only the six remaining path families (all page- or space-scoped, all carry an inline id) and explicitly call out the four exclusions. - Updates the Error cases table row to drop the "Known gap" pointer and describe the disallowed-path-family reason concretely. - Updates the Hard limits line in sandbox/agent-config/rules/environment.md to enumerate the four refused paths and direct callers to the narrow verbs (confluence search, confluence space list, confluence page footer-comments, confluence page inline-comments). Verified each enumeration against gateway/confluence_client.py:183-192 at branch HEAD (rebased onto 6b44b59). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * Address PR #2141 review: pagination, redaction, lock, allowlist Six fixes from review feedback on PR #2141: 1. populate_space_cache(): new method that walks _links.next pagination (capped at 4 pages) so /space/list cache lookups for spaceKey translation see all allowlisted spaces, not just the first 25. Preserves cursor semantics for the user-facing /space/list route by keeping list_spaces single-page. 2. Redact upstream error body: ConfluenceUpstreamError 5xx bodies are passed through redact_response before being included in the confluence_upstream_error response, so accountId/emailAddress leaked by Atlassian errors do not reach the sandbox. 3. Lock around lazy _client(): double-checked locking on ConfluenceClient.http_client construction to avoid two concurrent first-callers each creating a separate httpx.Client. 4. Drop descendants / comments / v1-comment paths from /execute allowlist: the dedicated narrow routes (/page/descendants, /page/footer-comments, /page/inline-comments) already enforce policy for those reads; /execute should not duplicate them. The remaining allowlist is just `api/v2/pages/{id}` and `api/v2/spaces/{id}/pages`. 5. Tighten CQL extractor rejection message for id/content/title clauses: now "id, content, and title clauses are not supported; use 'text ~ ...' instead" — points agents at the supported shape. 6. Rename _contains_top_level_or → _contains_or and _contains_bare_id_clause → _contains_id_clause to match the actual semantics (the regex matches at any depth, not just the top level; id/content/title are rejected with or without a space anchor). Tests: 24 new / updated assertions across test_confluence_client.py, test_confluence_routes.py, and test_confluence_search.py covering pagination, error-body redaction, lazy-client lock, and the new rejection-reason wording. Docs updated to reflect the tightened /execute allowlist. — Authored by egg * Address PR #2141 review observations: cursor comment + execute cache-miss test - Add inline comment to _extract_next_cursor explaining parse_qs blank-cursor fail-safe (observation #4 from egg-reviewer 997578d review). - Add direct route-level test for confluence_execute warming the paginated space cache via the api/v2/spaces/<id>/pages branch (observation #3 — the branch was previously only exercised indirectly through /space/pages). Author: egg <egg@localhost> * Catch ConfluenceUpstreamForbidden in confluence_execute cache warm Closes the blocking finding from the latest review on PR #2141: when populate_space_cache raises ConfluenceUpstreamForbidden during the space_id_in_path cache-miss branch (bot lacks space:read globally), the exception escaped as a Flask 500 instead of fail-closing through confluence_space_denied. ConfluenceUpstreamForbidden is a sibling of ConfluenceUpstreamError — both inherit directly from RuntimeError, not one from the other — so the existing exception tuple did not catch it. Mirrors the handler at _resolve_space_key_via_list. Also addresses two non-blocking observations: - Reword the _extract_next_cursor inline comment so it acknowledges the existing ``cursor or None`` guard rather than implying it would need to be added. - Extend the cache-miss test matrix for the space_id_in_path branch: - populate_space_cache raises ConfluenceUpstreamForbidden -> 403 - populate_space_cache succeeds but id stays unresolved -> 403 - resolved key is not in the operator allowlist -> 403 + audited key All three regression tests assert the upstream payload is not leaked to the agent on denial. Authored-by: egg * Reflect per-call-site 403 audit translation in docstring The ConfluenceUpstreamForbidden docstring claimed the route layer uniformly emits confluence_upstream_403 audit events, but in reality only confluence_space_pages does. _resolve_space_key_via_list (via confluence_search) and confluence_execute collapse the 403 into the route's *_denied event so allowlist resolution stays fail-closed and does not expose tenant-permission state. Updates the docstring to describe each call site's actual behaviour (reviewer follow-up option 2 from PR #2141 re-review). * Fix per-call-site 403 docstring to match all nine routes Reviewer flagged three accuracy issues in the ConfluenceUpstreamForbidden docstring on a7778f2: 1. confluence_search catches ConfluenceUpstreamForbidden directly and translates to confluence_upstream_403; the docstring incorrectly attributed it to _resolve_space_key_via_list. 2. Eight routes (not just confluence_space_pages) translate the 403 directly. The actual asymmetry is primary route call vs. auxiliary allowlist-resolution call. 3. The module-level docstring still claimed uniform translation, contradicting the new class docstring. Class docstring now enumerates all eight primary routes and the three auxiliary call sites that collapse the 403 (resolve_space_key_via_list, the parent re-fetch in descendants/footer-comments/inline-comments, and the execute cache-warm fallback). Module-level docstring redirects to the class docstring instead of repeating the inaccurate "all read methods" framing. --------- Co-authored-by: egg-orchestrator <egg@localhost> Co-authored-by: egg <egg@example.com> Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com> Co-authored-by: jwbron <8340608+jwbron@users.noreply.github.com> Co-authored-by: egg-reviewer[bot] <261018737+egg-reviewer[bot]@users.noreply.github.com> Co-authored-by: james-in-a-box[bot] <246424927+james-in-a-box[bot]@users.noreply.github.com>

jwbron mentioned this pull request Apr 27, 2026

tests: remove jira-wrapper artifact-location fallback (post-#2135 cleanup) #2136

Closed

egg-reviewer Bot approved these changes Apr 27, 2026

View reviewed changes