Add Confluence gateway wrapper with shared Atlassian creds (v1 read-only)

[james-in-a-box]

Sandboxed egg agents currently have no way to read Confluence
pages. The host-side mcp__confluence__* MCP that bundles
Atlassian (Jira + Confluence) is unreachable from the agent
container and exposes the operator's full Atlassian API surface
with no space or verb allowlist — violating egg's zero-credential
and "infrastructure beats config" invariants. The /impact-analysis
skill and the in-flight Jira-epic SDLC pipeline work (#1557) need
to read Confluence pages linked from Jira tickets during the refine
phase, and they cannot until this lands.

This PR adds read-only Confluence access through the existing
gateway sidecar, mirroring the /api/v1/jira/* pattern landed by
#1556 one component at a time:

1. Gateway foundation — new gateway/confluence_credentials.py
   (Atlassian API-token loader with mtime refresh, prefers a shared
   ATLASSIAN_* triple over per-service CONFLUENCE_* /
   back-compat JIRA_*, derives Confluence base URL by appending
   /wiki when only ATLASSIAN_BASE_URL is set); a small edit to
   gateway/jira_credentials.py so the same precedence applies to
   the Jira loader (ATLASSIAN_* preferred per key with JIRA_*
   fall-back) — this makes the shared-credential promise in
   decision F1 portable so operators can drop the legacy JIRA_*
   block after migration without breaking Jira;
   gateway/confluence_client.py (ConfluenceClient class backed
   by httpx, v2-first hybrid with v1 fallback for the known v2
   inline-comment 404 bug and v2 footer-comment nested-reply gap,
   single-retry on HTTP 429 honouring Retry-After, synthesised
   not_found envelope on 404 for read methods, distinct
   ConfluenceUpstreamForbidden(403) so operators can spot
   Atlassian permission denials separately, mandatory response
   redaction stripping accountId / emailAddress / _links.webui
   user-profile URLs, and a hardened validate_confluence_api_path
   regex allowlist that refuses write verbs, .. traversal,
   non-ASCII, and the permanent restrictions / permissions /
   space.admin / users / attachments denylist);
   gateway/confluence_policy.py (space-allowlist reader backed
   by a new confluence: section in config/context-filters.yaml
   — key is spaces; fail-closed on missing/malformed YAML); and
   gateway/confluence_search.py (conservative CQL extractor that
   deny-on-ambiguity rejects any CQL it cannot statically prove is
   scoped to allowlisted spaces — closes the regex-bypass path the
   analysis flagged).

2. Eight new routes plus a reload-hook extension in
   gateway/gateway.py —
   POST /api/v1/confluence/page/get,
   POST /api/v1/confluence/page/descendants,
   POST /api/v1/confluence/page/footer-comments (with optional
   --include-replies v1 fallback),
   POST /api/v1/confluence/page/inline-comments (transparent v1
   fallback on v2 404 with a used_fallback flag in the response),
   POST /api/v1/confluence/space/pages,
   POST /api/v1/confluence/space/list (response filtered to
   allowlisted spaces so agents cannot enumerate the full tenant
   set),
   POST /api/v1/confluence/search (backed by Atlassian's
   v1-only /wiki/rest/api/search, with the conservative
   space = / space IN (...) extractor),
   POST /api/v1/confluence/execute (GET-only, regex-allowlisted
   passthrough). _reload_all_config() is extended to call
   reload_confluence_credentials() and reload_confluence_policy().
   All eight routes are @require_session_auth +
   @require_private_mode + space-allowlist checked and produce
   structured audit logs; the new confluence_upstream_403 event
   distinguishes Atlassian permission denials from gateway-side
   allowlist denials.

3. Sandbox wrapper — new bash sandbox/scripts/confluence CLI
   wrapper exposing the eight gateway verbs as Jira-style
   subcommands (page get, page descendants,
   page footer-comments, page inline-comments, space pages,
   space list, search, execute) that calls the gateway with
   EGG_SESSION_TOKEN, mirroring sandbox/scripts/jira exactly.
   No per-pipeline env vars are added — Confluence is reference
   material, not a unit of work, and audits recover pageId /
   spaceKey from each request body.

4. Tests + docs + config scaffolding — unit + route + wrapper
   tests using the existing respx / fixture pattern (including
   429-retry, 404 / 403-envelope, v1 inline-comment fallback,
   footer-comment nested-reply merge, list_spaces filtering,
   response redaction, the 13+-case adversarial CQL suite, and the
   route-enumeration regression test); an extension to
   gateway/tests/test_allowed_domains.py adding
   wiki.atlassian.net / confluence.atlassian.com to the
   block-list parametrize; a new confluence: section in
   config/context-filters.yaml; a new # Atlassian (shared)
   block in config/secrets.template.env plus removal of the
   unused CONFLUENCE_SPACE_KEYS placeholder (replaced by the
   YAML allowlist); updates to
   docs/architecture/network-isolation.md,
   docs/architecture/credential-injection.md,
   sandbox/agent-config/rules/environment.md, and a new
   docs/reference/confluence-wrapper.md.

Impact. Sandboxed agents running in private network mode can
now read allowlisted Confluence spaces via the new confluence
wrapper. Atlassian credentials remain in the gateway exclusively
(zero additions to the sandbox env). Public-mode sessions cannot
reach Confluence — all eight routes return 403 before any upstream
call. The narrow verb surface plus the regex-allowlisted execute
escape hatch, combined with the permanent denylist (attachments,
restrictions, permissions, space.admin, users, DELETE / PUT /
PATCH), shape the code so the future writes scope (page/create,
page/update, comment/create) lands as three additional narrow
routes under the same decorator and policy plumbing, with no
re-architecting. Deferred to a future ticket: write idempotency
semantics (Q4), page/resolve-by-url (Q5), per-verb rate-limit
config, and any custom-macro PII redaction (Q3) once a real
payload is identified.

⚠️ Pre-merge Obligations

The reviewers below issued a conditional ACK — the work is approved, but a human must perform the listed action before merging. Do not merge this PR until every obligation is complete.

• reviewer_code_holistic — Resolve the conflict in shared/egg_restrictions/patterns.py by adopting main's version per main commit 2f693f3e9 / PR patterns: drop sandbox/scripts wholesale block, enforce via reviewer_security (#2133) #2135: drops the wholesale "sandbox/scripts/" entry from disallowed_patterns AND both the "sandbox/scripts/jira" and "sandbox/scripts/confluence" entries from block_exempt_patterns. The producer-branch hunk that adds "sandbox/scripts/confluence" is a no-op exemption against a block that no longer exists on main. Verified pre-merge: git merge origin/egg/issue-1931 from origin/main produces a CONFLICT on shared/egg_restrictions/patterns.py. After resolution, run pytest gateway/tests/test_agent_restrictions_*.py shared/tests/test_egg_restrictions.py gateway/tests/test_phase_filter_restrictions.py to confirm the matching test assertions (already on main since patterns: drop sandbox/scripts wholesale block, enforce via reviewer_security (#2133) #2135) pass.

Test Plan

• Automated (Phase 4):
  • gateway/tests/test_confluence_credentials.py — mtime refresh, missing-value error, base64 header shape, reload_confluence_credentials(); F1 precedence cases (ATLASSIAN_* alone with /wiki derivation, CONFLUENCE_* alone, mixed per-key fall-back).
  • gateway/tests/test_confluence_client.py — URL/header/body construction per method, default body-format=storage and override accepted, validate_confluence_api_path positive (every allowed family) + negative (restrictions, permissions, space.admin, users, attachments, DELETE, PUT, PATCH, .., duplicate slashes, non-ASCII, non-numeric pageId), pagination cursor round-trip across descendants/space-pages/list-spaces/search-cql, 429 single retry honouring Retry-After (writes never retry), 404 envelope on read methods (search_cql + execute_raw raise instead), ConfluenceUpstreamForbidden on 403, v1 inline-comment fallback on v2 404 (used_fallback flag), v2 footer-comment nested-reply merge when include_replies=True, list_spaces filtering excludes non-allowlisted entries, redact_response strips accountId / emailAddress / _links.webui user-profile URLs while preserving page _links.webui.
  • gateway/tests/test_confluence_policy.py — allowlist round-trip from confluence.spaces in tmp YAML, mtime reload, reload_confluence_policy(), missing file / missing section / missing spaces key / non-list shape / malformed YAML → empty set, mixed-case key preservation.
  • gateway/tests/test_confluence_search.py — positive (space = ENG, space IN (ENG, DOCS), combined with AND text ~ "RFC"); negative grid (space under OR, mixed key/id clauses, uppercase SPACE, quoted key, CQL functions, semicolons, comment markers, IN-list with non-allowlisted key, missing clause, unicode homoglyphs, bare id/title clauses).
  • gateway/tests/test_confluence_routes.py — for each of the eight routes, public-mode → 403 with private_mode_required audit, disallowed space → 403 (confluence__denied or confluence_space_denied), allowlisted happy path → 200 with body. 13+-case adversarial CQL suite end-to-end. /execute rejects write methods + denied verbs (restrictions, permissions, space.admin, users, attachments) + path traversal + disallowed spaces. Route-enumeration regression test asserts every /api/v1/confluence/ view function has egg_requires_private_mode=True. 404-envelope end-to-end. confluence_upstream_403 audit category surfaces for upstream 403s. list_spaces filtering end-to-end. redact_response end-to-end. used_fallback flag observable on inline-comment route.
  • tests/sandbox/test_confluence_wrapper.py — subprocess-invoke sandbox/scripts/confluence against a mock gateway; one happy + one failure path per verb (8 verbs); --include-replies and --depth toggles reach the request body.
  • gateway/tests/test_allowed_domains.py — extend parametrize list with wiki.atlassian.net + confluence.atlassian.com; existing assertion enforces atlassian.net / atlassian.com / api.atlassian.com / jira.atlassian.com remain absent (covers Confluence by extension).
• Manual:
  1. Fill ATLASSIAN_BASE_URL / ATLASSIAN_USERNAME / ATLASSIAN_API_TOKEN (or CONFLUENCE_* equivalents) in ~/.config/egg/secrets.env and add a space to config/context-filters.yaml :: confluence.spaces. Confirm the bot has read scope on that space in Atlassian's UI.
  2. Start the gateway in private mode and curl each of the eight routes with an allowlisted page; confirm JSON responses with body.storage populated and accountId / emailAddress redacted.
  3. Start in public mode; confirm every Confluence route returns 403 with private_mode_required.
  4. Call /api/v1/confluence/execute with method=DELETE and with path=api/v2/pages/123/restrictions; confirm 403 with confluence_execute_denied.
  5. Call /api/v1/confluence/search with CQL "space = ENG OR space = SEC" (SEC not allowlisted); confirm 403 confluence_search_rejected.
  6. Call /api/v1/confluence/page/get with a non-existent pageId in an allowlisted space; confirm HTTP 200 with a not_found envelope body.
  7. Call /api/v1/confluence/space/list; confirm the response contains only allowlisted spaces (no leak of the full tenant space set).
  8. Force /api/v1/confluence/page/inline-comments to hit the v2 404 bug (or use a test page known to trigger it); confirm the response contains used_fallback=true and the v1 payload.
  9. From inside a sandbox container, run confluence page get, confluence search, confluence space list; confirm JSON returned.
  10. env inside the sandbox shows no ATLASSIAN_* / CONFLUENCE_* / JIRA_* keys — only EGG_SESSION_TOKEN and the GATEWAY_URL.
  11. POST /api/v1/config/reload; confirm audit log shows confluence_config_reloaded fired.

Manual Steps

Pre-merge:

• Operator confirms the Atlassian bot account already used for Jira has read scope on every space being allowlisted (Atlassian: Space settings → Permissions → at least View for the bot user).
• Operator decides which Confluence spaces to allowlist and edits config/context-filters.yaml :: confluence.spaces before enabling the feature in production. Empty list is valid; keeps feature installed-but-inert.
• Operator confirms *.atlassian.net is NOT in the Squid domain allowlist (gateway/allowed_domains.txt). The extended test in Task 4-7 enforces this for Confluence-named hostnames.
• If migrating off independent JIRA_* / CONFLUENCE_* triples to the shared ATLASSIAN_* triple, copy the same value into all three triples during the cutover; both gateway/jira_credentials.py (updated by Task 1-5) and gateway/confluence_credentials.py prefer ATLASSIAN_* per key, so removing the legacy blocks once ATLASSIAN_* is fully populated is safe.
  Post-merge:
• Roll the gateway pod so it picks up the new secrets.env values and the updated context-filters.yaml, or POST /api/v1/config/reload for a hot-reload.
• Execute the manual verification steps from the test plan against the live gateway.
• Notify owners of Add SDLC pipeline support for Jira epics #1557 (Jira-epic SDLC pipelines) and the /impact-analysis skill that the Confluence wrapper is available and unblocks their pipeline integration.

Pipeline Context

Pipeline: issue-1931
Issue: #1931

Per-phase BRC transcripts: implement.

Authored-by: egg

[james-in-a-box]

Conflict Resolution Summary

Resolved merge conflicts with main:

File	Category	Resolution
shared/egg_restrictions/patterns.py	Semantic (additive on this side, removed on main)	Adopted main's version per #2135 (commit 2f693f3e9) — dropped both "sandbox/scripts/jira" and "sandbox/scripts/confluence" exemptions from CODER_PATTERNS.block_exempt_patterns. The wholesale "sandbox/scripts/" block was already removed on main, so the exemptions were no-ops; sandbox-script writes are now policed by reviewer_security instead of a static blocklist.

This matched reviewer_code_holistic's pre-merge obligation exactly.

Verification (local):

• pytest gateway/tests/test_agent_restrictions_*.py shared/tests/test_egg_restrictions.py gateway/tests/test_phase_filter_restrictions.py — 653 passed
• pytest gateway/tests/test_confluence_*.py gateway/tests/test_allowed_domains.py — 210 passed
• pytest tests/sandbox/test_confluence_wrapper.py tests/sandbox/test_jira_wrapper.py — 43 passed
• pytest gateway/tests/ shared/tests/ (full sweep) — 3733 passed, 26 skipped
• ruff check shared/egg_restrictions/patterns.py — clean

Please review: Nothing surprising — the conflict was the exact one the holistic reviewer flagged, and the resolution is the one they specified. The Confluence wrapper file (sandbox/scripts/confluence) added by this PR is now permitted under the catch-all coder allowlist with no special-case exemption, which is the intended end state per #2135.

— Authored by egg

[egg-reviewer]

No agent-mode design concerns.

This is pure infrastructure (gateway routes + sandbox CLI wrapper, mirroring the existing Jira pattern) and aligns well with the agent-mode principles:

• Tool, not pre-fetch: The confluence wrapper gives agents a way to pull what they need on demand. Nothing is baked into prompts.
• Sandbox-enforced constraints: The space allowlist, permanent denylist (restrictions / permissions / users / attachments), write-verb refusal, CQL scope extractor, redaction, and private-mode gating are all enforced at the gateway — not via prompt-level instructions.
• No structured-output coercion of agent output, no post-processing pipeline, and no orchestrator-exported per-pipeline env var (the doc explicitly notes Confluence is reference material, not a unit of work).
• No direct LLM API calls introduced; no hardcoded model identifiers.

The "no per-pipeline env var" decision is particularly well-aligned — it leaves the agent free to decide which page/space to read based on the task, rather than constraining it to a pre-selected scope.

— Authored by egg

[james-in-a-box]

Contract Verification — Approve

Contract state

• current_phase: refine; acceptance_criteria: []; phases: []; pr: null.
• 15/15 multiple-choice decisions and the 10-question feedback are resolved.
• The plan is not lifted into the structured contract — it lives in .egg-state/drafts/1931-plan.md (Phase 1–6, Tasks 1-1 through 6-4) plus .egg-state/agent-outputs/1931-architect-output.json and 1931-risk_analyst-output.json.
• I cannot call egg-contract verify-criterion because (a) no ac-N entries exist and (b) the orchestrator is unreachable from this session. I instead verified each plan task against the implementation manually below.

Decision/feedback alignment

Every resolved decision has a matching artefact in the diff:

Decision	Resolution	Evidence
1 — Endpoint surface	Jira-style verb-noun paths	gateway/gateway.py:5016-5712 (eight /api/v1/confluence/* POST routes)
2 — API version	v2-first hybrid + v1 CQL + v1 fallback	gateway/confluence_client.py per-method endpoint pinning
3 — CQL scope	Conservative static extractor, deny-on-ambiguity	gateway/confluence_search.py:extract_search_spaces
4 — Comment quirks	v2 + transparent v1 fallback	confluence_client.py::get_page_inline_comments (v2→v1 on 404), footer-comment merge for nested replies
5 — Body format	storage only by default; override allowed	DEFAULT_BODY_FORMAT = ("storage",); _validate_body_format accepts atlas_doc_format, view, export_view
6 — Credentials	Shared ATLASSIAN_* triple, per-key CONFLUENCE_*/JIRA_* fallback	gateway/confluence_credentials.py::_load_credentials; symmetric edit in gateway/jira_credentials.py
7 — Network gate	@require_private_mode per route + enumeration regression test	All eight handlers carry the decorator; TestRouteEnumeration::test_every_confluence_route_has_private_mode_marker walks app.url_map and asserts __egg_requires_private_mode__ = True
8 — Allowlist location	confluence.spaces in config/context-filters.yaml	YAML stub added with spaces: []
9 — Bot identity	Same Atlassian bot as Jira	Implicit in shared-credential design
10 — Redaction	Strip accountId, emailAddress, user-profile _links.webui	confluence_client.py::redact_response recursive walker
11 — space/list filter	Filter to allowlist	ConfluenceClient.list_spaces(allowed_spaces=...) filters before return
12 — Attachments	Permanent denylist	CONFLUENCE_DENIED_VERBS includes attachments, restrictions, permissions, space.admin, users, DELETE, PUT, PATCH, POST
13 — EGG_CONFLUENCE_*	None in v1	No env keys added; _session_confluence_context() per-call
14 — /execute	Include, GET-only, regex-allowlisted	gateway/gateway.py:5703-5712 + tightened allowlist (see Notable deviation below)

Free-form feedback Q1–Q10 also align: empty allowlist, single-retry on 429, default redaction set, no write idempotency, no resolve-by-url verb, no depth cap, separate confluence_upstream_403 audit category, current-version-only reads, bot-vs-human caveat documented in docs/reference/confluence-wrapper.md, Jira-style subcommand shape only.

Phase-by-phase task verification

Phase 1 — Gateway foundation (Tasks 1-1…1-5): all five files present.

• confluence_credentials.py (289 lines): F1 precedence + /wiki derivation when only ATLASSIAN_BASE_URL is set; raises typed ConfluenceCredentialsUnavailable; reload_confluence_credentials() exported.
• confluence_client.py (1090 lines): per-verb endpoint pinning; validate_confluence_api_path regex allowlist; 429 retry honouring Retry-After; 404 envelope on read methods; ConfluenceUpstreamForbidden for 403; v1 inline-comment fallback; redact_response recursive walk; _SpaceCache LRU 60s TTL; CONFLUENCE_RESPONSE_MAX_BYTES = 5 MiB.
• confluence_policy.py (244 lines): mtime cache, fail-closed on missing/malformed YAML, reload_confluence_policy() exported.
• confluence_search.py (212 lines): parse-then-validate; rejects OR, bare id/content/title, SPACE casing, quoted keys, function-call values, ;, /* */, --, //, non-ASCII; IN (...) requires every key allowlisted.
• jira_credentials.py edit: per-key ATLASSIAN_* precedence with JIRA_* fallback; no /wiki derivation (Jira lives at bare origin).

Phase 2 — Gateway routes (Tasks 2-1…2-9): eight routes wired in gateway/gateway.py:5016-5712; _reload_all_config() extended at line 976-996 with confluence_config_reloaded audit covering both reloads.

Phase 3 — Sandbox wrapper (Task 3-1): sandbox/scripts/confluence (bash, set -euo pipefail) implements all eight Jira-style subcommands: page get/descendants/footer-comments/inline-comments, space pages/list, search, execute, help. Bearer auth with EGG_SESSION_TOKEN, JSON-on-stdout, errors-on-stderr.

Phase 4 — Tests (Tasks 4-1…4-7): all six new test files present, plus test_jira_credentials.py and test_allowed_domains.py extended. All 255 tests pass locally:

• test_confluence_credentials.py: 19 tests
• test_confluence_policy.py: 17 tests
• test_confluence_search.py: 35 tests (covers every adversarial case from the plan grid)
• test_confluence_client.py: 82 tests
• test_confluence_routes.py: 49 tests (incl. TestRouteEnumeration, anti-bypass via /execute, redaction E2E, confluence_upstream_403 distinct audit, used_fallback propagation, audit-shape coverage)
• test_jira_credentials.py: 21 tests (extended for ATLASSIAN_* precedence matrix)
• test_allowed_domains.py: 8 tests (wiki.atlassian.net, confluence.atlassian.com added to parametrize list, plus Confluence-naming docstring)
• tests/sandbox/test_confluence_wrapper.py: 24 tests (one happy + one failure per verb, --include-replies and --depth arg propagation)

Phase 5 — Config + k8s (Tasks 5-1…5-3): config/context-filters.yaml adds confluence.spaces: [] (fail-closed); config/secrets.template.env adds ATLASSIAN_* block above the JIRA_*/CONFLUENCE_* legacy blocks, drops CONFLUENCE_SPACE_KEYS, points operators at the YAML; k8s/base/gateway-deployment.yaml adds inline comments for the new env keys (no new volumes).

Phase 6 — Documentation (Tasks 6-1…6-4): all four edits present.

• docs/architecture/network-isolation.md: /api/v1/confluence/* block with eight routes added next to Jira.
• docs/architecture/credential-injection.md: Confluence row added; precedence + /wiki derivation documented.
• sandbox/agent-config/rules/environment.md: full confluence wrapper section with all eight verbs.
• docs/reference/confluence-wrapper.md (346 lines): full reference incl. not_found envelope, redaction, v1 fallback used_fallback, Q9 bot-vs-human caveat, Atlassian rate-limit runbook, last-reviewed footer (2026-04-27). docs/index.md lookup-row added.

Notable deviation from plan (security tightening)

The plan listed five /execute allowlist patterns; the implementation ships six narrow page-scoped patterns and intentionally omits four flat ones:

• Removed: rest/api/search, api/v2/spaces, api/v2/footer-comments (flat), api/v2/inline-comments (flat).
• Reason (per the inline comment at gateway/confluence_client.py:166-188): each removed family is already covered by a narrow route and /execute-routing them would skip the route-level safeguards (CQL extractor, allowlist filter, post-fetch space check). Mirrors PR #1964's anti-bypass denylist on Jira.
• Tested in test_confluence_routes.py::TestExecute::test_anti_bypass_paths_rejected_via_execute.

This is a stricter posture than the plan, motivated by reviewer_code + reviewer_security cycle-3 NACKs, and the deviation is explicitly documented in code. Acceptable — narrows the attack surface without losing functionality (every removed path is reachable via its narrow route).

Contract integrity gap (informational, not blocking this PR)

The structured contract has empty acceptance_criteria and phases arrays even though six phases of work shipped. This is a process gap in how the refine→plan→implement transitions populate the contract for this pipeline — implementation traceability lives in the draft markdown rather than in phases.<p>.tasks.<t> entries with linked commits. Worth raising as a follow-up for the SDLC tooling, but the implementation itself is complete and well-aligned with the plan, so it does not block approval.

Verdict

Approve. All 14 decisions, all 10 feedback answers, and all six plan phases are reflected in the diff with no functional gaps. The /execute deviation is a documented security improvement. Test suite (255 tests) is green.

— Authored by egg

[james-in-a-box]

Contract Verification Re-review (incremental)

Range reviewed: e1e0e37…c857bf5 (1 commit, 3 files changed: +101 / -4).

Contract state (unchanged since last review)

• current_phase: refine; acceptance_criteria: []; phases: []; pr: null.
• 15/15 multiple-choice decisions and the 10-question feedback are still resolved.
• The plan is still living in .egg-state/drafts/1931-plan.md (Phase 1–6, Tasks 1-1 through 6-4) and has not been lifted into the structured contract.
• I therefore still have no ac-N to call egg-contract verify-criterion against. No regression on previously-verified criteria is possible because there are no previously-verified criteria.

Delta summary

File	Δ	Nature
gateway/gateway.py	+11 / −2	confluence_execute cache-warm branch now catches ConfluenceUpstreamForbidden
gateway/confluence_client.py	+4 / −2	_extract_next_cursor comment reworded
gateway/tests/test_confluence_routes.py	+86 / 0	Three regression tests for the cache-miss space_id_in_path branch

Verification of the bug fix flagged in the previous code review

Previous code reviewer (egg-reviewer at e1e0e37) flagged a blocking issue: when populate_space_cache() raises ConfluenceUpstreamForbidden during the space_id_in_path cache-miss branch, the existing (ConfluenceCredentialsUnavailable, ConfluenceUpstreamError) tuple did not catch it, so the exception escaped as a Flask 500 instead of fail-closing through confluence_space_denied.

I confirmed the inheritance claim: gateway/confluence_client.py:220 defines ConfluenceUpstreamError(RuntimeError) and :232 defines ConfluenceUpstreamForbidden(RuntimeError) — siblings, not parent/child, so the original tuple genuinely did not catch the 403 path.

The fix at gateway/gateway.py:5871-5878 adds ConfluenceUpstreamForbidden to the exception tuple, mirroring the handler at _resolve_space_key_via_list. The cache-miss flow now correctly returns 403 + confluence_execute_denied instead of 500.

The three new regression tests cover the full 403 fail-closed matrix for this branch:

• test_space_id_path_warm_403_fails_closed — populate_space_cache raises ConfluenceUpstreamForbidden → 403, no upstream payload leaked
• test_space_id_path_warm_unresolved_fails_closed — populate_space_cache succeeds but the id remains unresolved → 403 + space_key=None
• test_space_id_path_warm_resolves_to_disallowed_space — id resolves to a non-allowlisted key → 403 + audited resolved key + no payload leak

All three tests pass locally (pytest gateway/tests/test_confluence_routes.py::TestExecute::test_space_id_path_warm_* → 3 passed).

Decision-level compliance check (delta only)

The single new commit only touches gateway code. None of the resolved decisions are violated:

• decision-7 (@require_private_mode per-route): confluence_execute continues to use the existing decorator; the change is inside the route body. No regression.
• decision-8 (allowlist in confluence.spaces:): the fix preserves fail-closed allowlist behaviour — denial still routes through _confluence_space_denied_response with audited_space_key populated when known.
• decision-10 (response redaction): denial path returns the standard denial envelope (no upstream body); the regression tests assert "leak-bait" is not in the response body for all three failure modes.
• All other decisions are infrastructure / config-shape and are not touched by this delta.

Verdict

No acceptance criteria to verify, no contract violations introduced by the delta, and the only blocking finding from the previous code review is correctly fixed with regression coverage. Posting as a comment per convention for refine-phase contracts that carry no ac-N entries.

— Authored by egg

[egg-reviewer]

Re-review: PR #2141 (delta e1e0e37 → c857bf5)

Reviewed the delta from e1e0e37 → c857bf5 ("Catch ConfluenceUpstreamForbidden in confluence_execute cache warm"). The blocking finding from the previous review is properly fixed, the comment wording nit is addressed, and the test matrix is meaningfully widened. Approving with one non-blocking observation worth a follow-up.

Verification of previous blocking finding

Prev. finding	Status	Evidence
Blocking — confluence_execute does not catch ConfluenceUpstreamForbidden from populate_space_cache	Fixed	gateway/gateway.py:5872-5878 now catches all three exception types: ConfluenceCredentialsUnavailable, ConfluenceUpstreamError, ConfluenceUpstreamForbidden. The exception-class hierarchy is correct — both Confluence*Error and Confluence*Forbidden inherit directly from RuntimeError (gateway/confluence_client.py:220, :232), neither subclassing the other, so the prior tuple genuinely missed the 403 case. The new inline comment spells out the why (sibling, not subclass; would otherwise escape as Flask 500; mirrors _resolve_space_key_via_list) — exactly what I asked for.

All three call sites of populate_space_cache() now handle 403 consistently with the route's needs:

• _resolve_space_key_via_list (gateway.py:4975-4980) → fail-closed via confluence_space_denied
• confluence_space_pages (gateway.py:5521-5527) → distinct confluence_upstream_403 audit event (operator-facing endpoint)
• confluence_execute (gateway.py:5872-5878) → fail-closed via confluence_execute_denied (this PR)

Verification of non-blocking observation #4

gateway/confluence_client.py:1021-1026 — comment now correctly acknowledges the existing cursor or None guard at line 1032 rather than implying it would need to be added. Wording is precise.

New test coverage

Three new regression tests in TestExecute close the matrix gap I called out:

1. test_space_id_path_warm_403_fails_closed — covers the blocking fix directly. populate_space_cache.side_effect = ConfluenceUpstreamForbidden(...); asserts HTTP 403, confluence_execute_denied audit, and "leak-bait" upstream payload absent from the response.
2. test_space_id_path_warm_unresolved_fails_closed — warm succeeds but id stays unresolved; asserts populate_space_cache.assert_called_once(), 403, audit spaceKey=None, and payload not leaked.
3. test_space_id_path_warm_resolves_to_disallowed_space — warm resolves to SECRET which is not in the allowlist; asserts 403, audit spaceKey="SECRET" (operator can debug the denial), and payload not leaked.

The "leak-bait" assertions in all three are a particularly nice touch — they prove that even when the upstream call has already returned data, the gateway does not surface it on a denial.

Non-blocking — operator-observability inconsistency

The fix collapses the 403-from-populate_space_cache case into confluence_execute_denied with spaceKey=None. That is consistent with _resolve_space_key_via_list and was exactly the pattern I recommended last review, so the implementation matches the request. However, it leaves a documented inconsistency in the audit-event surface:

• ConfluenceUpstreamForbidden's docstring at gateway/confluence_client.py:232-238 explicitly promises: "The route layer translates this to a confluence_upstream_403 audit event so operators can distinguish bot-account permission denials from space-allowlist denials and other upstream errors (Q7, risk R15)."
• confluence_space_pages honors that contract (gateway.py:5524-5526 → _confluence_forbidden_response(..., event="confluence_upstream_403", ...)).
• _resolve_space_key_via_list and now confluence_execute do not — both translate the 403 into confluence_*_denied with spaceKey=None.

In a real tenant where the bot loses space:read globally, the operator looking at audit logs sees confluence_execute_denied events with spaceKey=None and no way to distinguish "bot lost global permissions" (an ops fire) from "agent sent a bogus spaceId" (benign). The Q7/R15 audit-event story falls back to the upstream-call layer for those routes, which the audit pipeline doesn't surface as cleanly.

Possible follow-ups (any one is fine, no need to do it in this PR):

• Emit a separate confluence_upstream_403 event from the except ConfluenceUpstreamForbidden: branch at gateway.py:5872-5878 and gateway.py:4975-4980, in addition to the fail-closed *_denied event the route returns. The agent-facing response stays unchanged; ops gets the discriminator they expect.
• Or update the ConfluenceUpstreamForbidden docstring to acknowledge that two of the three call sites collapse the 403 into a denial event (so the contract matches reality).

Not blocking — the fail-closed semantics are correct and the agent-facing behavior is right. This is purely about operator observability for a documented Atlassian failure mode.

Verdict

Comment (self-authored PR; the gh wrapper would auto-downgrade --approve here anyway, but the intent is approval). The blocking issue is fixed cleanly, with regression tests that exercise the exact branch the prior review caught, plus two adjacent branches that close the test matrix.

— Authored by egg

[james-in-a-box]

Pushed a7778f2 addressing the non-blocking observation from the latest re-review by going with option 2 (docstring-only).

The ConfluenceUpstreamForbidden docstring at gateway/confluence_client.py:232 now describes the actual per-call-site behaviour:

• confluence_space_pages → confluence_upstream_403 (operator-facing endpoint, distinct event preserved).
• _resolve_space_key_via_list (via confluence_search) and confluence_execute → confluence_*_denied with space_key=None, because the 403 surfaces during allowlist resolution and the route's contract is to deny rather than expose tenant-permission state. The docstring explicitly notes that operators investigating a global space:read loss can cross-reference these denials with HTTP-client-layer 403 logs.

Skipped option 1 (emit a separate confluence_upstream_403 in addition to the *_denied event) for this PR — the reviewer flagged it as a follow-up and dual-emitting from a fail-closed branch deserves its own ticket so the audit-pipeline owners can sign off on the event-cardinality change. Happy to open one if it would help.

Tests: pytest gateway/tests/test_confluence_client.py gateway/tests/test_confluence_routes.py → 145 passed; ruff check clean.

— Authored by egg

[egg-reviewer]

No agent-mode design concerns in the delta.

The single new commit (a7778f2) is a docstring-only clarification on ConfluenceUpstreamForbidden in gateway/confluence_client.py, documenting the per-call-site 403 audit translation behavior. No agent-facing surface, prompt, or tool contract changes.

— Authored by egg

[james-in-a-box]

Re-review: contract delta for a7778f26b49070f057c4ea7cda80e40c0d92ee1b (docstring-only).

Delta since prior review at c857bf5fd938b9ecc6f7cbf14c49803761f96173: one commit, gateway/confluence_client.py only (+14/-3).

Contract state

The pipeline contract (.egg-state/contracts/issue-1931.json) carries acceptance_criteria: [] and phases: [] — only HITL decisions and feedback are populated. There are no contract criteria or tasks to mark verified via egg-contract verify-criterion, so this re-review is purely a regression check on the docstring change.

Verification of the docstring claims

The new ConfluenceUpstreamForbidden docstring asserts per-call-site translation. I cross-referenced the three call sites against gateway/gateway.py on the PR head:

1. confluence_space_pages → confluence_upstream_403 (direct). Confirmed at gateway.py:5524-5527 (during populate_space_cache) and gateway.py:5550-5552 (during get_space_pages). Both except ConfluenceUpstreamForbidden arms call _confluence_forbidden_response(exc, event="confluence_upstream_403", space_key=space_key).

2. _resolve_space_key_via_list (used by confluence_search) → collapses to confluence_*_denied. Confirmed at gateway.py:4972-4985: the helper catches ConfluenceUpstreamForbidden alongside other upstream errors and returns None. The post-fetch allowlist check at gateway.py:5035-5043 then fail-closes to False when space_key is None, which triggers the route's confluence_*_denied event with space_key=None — matching the docstring's "fail-closed … does not expose tenant-permission state" claim.

3. confluence_execute → confluence_execute_denied. Confirmed at gateway.py:5862-5887. When client.populate_space_cache() raises ConfluenceUpstreamForbidden inside the resolve-by-spaceId block, resolved stays None, which falls through to _confluence_space_denied_response(event="confluence_execute_denied", space_key=resolved, ...) at gateway.py:5882-5887. The audit event is confluence_execute_denied, not confluence_upstream_403, exactly as the new docstring states.

Regressions

None. The change is documentation-only; no production code, audit-event names, or fail-closed behavior changed since c857bf5. Previously verified behavior (allowlist enforcement, audit-event taxonomy, per-route 403 handling) is unaffected.

Verdict

Approve. The docstring now accurately describes the call-site-specific 403→audit-event translation in gateway/gateway.py. Contract has no acceptance criteria to mark verified.

— Authored by egg

[egg-reviewer]

Re-review: PR #2141 (delta c857bf5 → a7778f26)

Reviewed the docstring-only delta. Single commit, single file:

gateway/confluence_client.py +14 -3

The change rewrites the ConfluenceUpstreamForbidden class docstring at gateway/confluence_client.py:232-249 to describe per-call-site 403 audit translation, addressing my prior non-blocking option-2 follow-up.

Approving — no functional change, no test impact, no security/correctness implications. But the new docstring still doesn't fully match the code, so I'm leaving non-blocking observations for a follow-up.

Non-blocking — docstring still does not fully match reality

The previous review's ask was that the docstring "match reality." The new wording is closer but has three accuracy issues. They're all in the same paragraph the author just edited, so calling them out:

1. _resolve_space_key_via_list is not used by confluence_search

gateway/confluence_client.py:241 says:

_resolve_space_key_via_list (used by confluence_search) and confluence_execute collapse the 403 …

_resolve_space_key_via_list is defined at gateway/gateway.py:4954 and called from a single site: _check_post_fetch_space_allowlist (gateway.py:5035). That helper is in turn called from:

• confluence_page_get (gateway.py:5115)
• confluence_page_descendants parent re-fetch (gateway.py:5221)
• confluence_page_footer_comments parent re-fetch (gateway.py:5327)
• confluence_page_inline_comments parent re-fetch (gateway.py:5437)
• confluence_execute post-fetch check (gateway.py:5850)

confluence_search (gateway.py:5640-5728) catches ConfluenceUpstreamForbidden from client.search_cql(...) directly at gateway.py:5700-5701 and translates it to confluence_upstream_403. It never invokes _resolve_space_key_via_list. The parenthetical attribution is wrong.

2. Singling out confluence_space_pages as "the" route that emits confluence_upstream_403 is misleading

The new docstring frames the asymmetry as "confluence_space_pages does direct translation; the others collapse." Reality is: nine route-level except ConfluenceUpstreamForbidden handlers translate to confluence_upstream_403:

gateway.py:5089   confluence_page_get
gateway.py:5191   confluence_page_descendants (primary fetch)
gateway.py:5300   confluence_page_footer_comments (primary fetch)
gateway.py:5409   confluence_page_inline_comments (primary fetch)
gateway.py:5526   confluence_space_pages (cache-warm)
gateway.py:5552   confluence_space_pages (get_space_pages)
gateway.py:5607   confluence_space_list
gateway.py:5701   confluence_search (primary)
gateway.py:5830   confluence_execute (primary execute_raw)

The actual asymmetry is primary route call vs. auxiliary allowlist-resolution call, not confluence_space_pages vs. the rest. The auxiliary calls that collapse the 403 are:

• _resolve_space_key_via_list (gateway.py:4979) — silent catch, returns None, route emits confluence_*_denied with space_key=None
• Parent re-fetch in descendants / footer-comments / inline-comments (gateway.py:5217, 5323, 5433) — silent catch, parent set to None, route emits confluence_space_denied with space_key=None
• confluence_execute cache-warm in the space_id_in_path branch (gateway.py:5876) — silent catch, route emits confluence_execute_denied with space_key=None

The third bullet (parent re-fetch in the three comment/descendant routes) isn't mentioned in the new docstring at all, and it's structurally identical to the cache-warm collapse the docstring does call out.

3. The module-level docstring still claims uniform translation

gateway/confluence_client.py:62-66 (unchanged by this PR) still says:

403 envelope (Q7, risk R15):

• All read methods raise ConfluenceUpstreamForbidden on upstream 403 so the route layer can audit it as confluence_upstream_403 (distinct from generic upstream errors).

That's the original overgeneralization the new class docstring was meant to correct, and it survived the edit. Two contradicting docstrings in the same file is worse than one wrong-but-consistent docstring.

Suggested wording

Something like:

Route-layer translation is per-call-site (Q7, risk R15):

• Primary route calls (confluence_page_get, confluence_page_descendants, confluence_page_footer_comments, confluence_page_inline_comments, confluence_space_pages, confluence_space_list, confluence_search, confluence_execute) translate this directly to confluence_upstream_403 so operators can distinguish bot-account permission denials from other upstream errors.
• Auxiliary allowlist-resolution calls — _resolve_space_key_via_list (used by the post-fetch space-allowlist check), the parent-page re-fetch inside descendants / footer-comments / inline-comments, and the cache-warm fallback inside confluence_execute's space_id_in_path branch — collapse the 403 into the route's fail-closed confluence_*_denied event with space_key=None. The 403 surfaces during allowlist resolution and the route's contract is to deny rather than expose tenant-permission state.

…and update the module-level docstring at lines 62-66 to point to this class docstring instead of repeating the (now-incorrect) "all read methods" framing.

Verification

• Diff scope confirmed via gh api repos/jwbron/egg/compare/c857bf5...a7778f26: exactly one file (gateway/confluence_client.py, +14/-3), docstring-only.
• No tests changed; behavior identical to c857bf5. The new tests added in that prior commit (test_space_id_path_warm_403_fails_closed etc.) still pass against this delta — no edits required.
• Ran sanity check on call-site enumeration: grep -n ConfluenceUpstreamForbidden gateway/gateway.py gives 18 hits, including 9 except-handler sites whose event= arguments I enumerated above.

Verdict

Approve. Documentation accuracy follow-ups are non-blocking — the wrapper's behavior is correct (and was correct before this commit). Worth a one-line follow-up commit if convenient.

— Authored by egg

[james-in-a-box]

Pushed 9eed744 addressing the three docstring-accuracy observations:

1. confluence_search attribution. Removed — confluence_search catches ConfluenceUpstreamForbidden directly at gateway.py:5700 and emits confluence_upstream_403. The class docstring no longer claims _resolve_space_key_via_list is on its path.
2. Asymmetry framing. Class docstring now enumerates all eight primary routes (confluence_page_get, confluence_page_descendants, confluence_page_footer_comments, confluence_page_inline_comments, confluence_space_pages, confluence_space_list, confluence_search, confluence_execute) that translate to confluence_upstream_403 directly, and all 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 confluence_execute cache-warm fallback in the space_id_in_path branch).
3. Module-level docstring contradiction. Lines 62-67 no longer repeat "all read methods … audit it as confluence_upstream_403"; they now redirect to ConfluenceUpstreamForbidden for the per-call-site taxonomy.

145 confluence client + route tests pass; ruff clean.

— Authored by egg

[egg-reviewer]

Re-review: PR #2141 (delta a7778f26 → 9eed7442)

Single commit since prior review (9eed744), docstring-only refinement to gateway/confluence_client.py:

• Module docstring: clarifies that route-layer 403 translation is per-call-site, with a pointer to ConfluenceUpstreamForbidden for the full taxonomy.
• ConfluenceUpstreamForbidden docstring: enumerates all nine primary route call sites and the auxiliary allowlist-resolution call sites that collapse 403s into fail-closed denied events.

No code changes, no prompt or agent-surface changes. Documentation-only delta with no agent-mode design implications.

No agent-mode design concerns.

— Authored by egg

[james-in-a-box]

Re-review verdict: approve

Delta since previous review (a7778f2): a single commit (9eed744) — docstring-only edit to gateway/confluence_client.py (+18/−8). No code, test, or config changes.

Verification of the new docstring

I cross-checked every claim in the rewritten ConfluenceUpstreamForbidden class docstring against gateway/gateway.py:

8 primary routes that translate ConfluenceUpstreamForbidden → confluence_upstream_403:

Route	Catch site
confluence_page_get	gateway/gateway.py:5088-5089
confluence_page_descendants	gateway/gateway.py:5190-5191
confluence_page_footer_comments	gateway/gateway.py:5299-5300
confluence_page_inline_comments	gateway/gateway.py:5408-5409
confluence_space_pages	gateway/gateway.py:5550-5552 (also 5524-5527 for the spaceKey→spaceId cache populate; same translation, both consistent)
confluence_space_list	gateway/gateway.py:5606-5607
confluence_search	gateway/gateway.py:5700-5701
confluence_execute	gateway/gateway.py:5829-5830

3 auxiliary allowlist-resolution sites that collapse to confluence_*_denied with space_key=None:

• _resolve_space_key_via_list (invoked by _check_post_fetch_space_allowlist at gateway/gateway.py:5031-5040) — collapses via the route's normal not-allowlisted path.
• Parent-page re-fetch inside confluence_page_descendants / confluence_page_footer_comments / confluence_page_inline_comments — gateway/gateway.py:5217-5232, 5323-5344, 5433-5454. All catch ConfluenceUpstreamForbidden and return confluence_space_denied.
• confluence_execute cache-warm fallback in the space_id_in_path branch — gateway/gateway.py:5871-5888. Catches ConfluenceUpstreamForbidden alongside ConfluenceUpstreamError, leaves resolved=None, returns confluence_execute_denied.

The three accuracy fixes the commit message calls out are all present:

1. confluence_search is now correctly listed under primary routes (it catches ConfluenceUpstreamForbidden directly at line 5700-5701, not via _resolve_space_key_via_list).
2. The class docstring correctly enumerates eight primary routes (not just confluence_space_pages).
3. The module-level 403 envelope paragraph now redirects to the class docstring (gateway/confluence_client.py:62-67) instead of repeating the inaccurate "all read methods" framing.

Contract impact

No regression. The change is comment-only:

• No previously verified acceptance criterion can be broken — the modified file produces the same byte stream after compilation modulo docstring text.
• All eight Confluence routes still translate upstream 403s as documented; auxiliary sites still collapse as documented.
• No test / fixture changes were needed; existing audit-event tests still cover the behaviour.

Pre-merge obligations

The conditional-ACK obligation from the previous review cycle still applies and is not addressed in this delta — the producer still needs the human merger to resolve the shared/egg_restrictions/patterns.py conflict against main's 2f693f3e9 / PR #2135 (drop the "sandbox/scripts/" block entry plus both block_exempt_patterns exemptions for jira and confluence). This was already documented in the PR body's "⚠️ Pre-merge Obligations" section, so flagging again only as a reminder — it is not a regression introduced by 9eed744.

— Authored by egg

[james-in-a-box]

Automated feedback loop has reached the maximum of 5 rounds. Human review is needed to make further progress on this PR.

[james-in-a-box]

egg contract-verification completed. View run logs

26 previous review(s) hidden.

[egg-reviewer]

Re-review: PR #2141 (delta a7778f26 → 9eed7442)

Single docstring-only commit (9eed744, gateway/confluence_client.py +18/−8). All three accuracy issues from my previous review are correctly addressed.

Verification of the three fixes

1. confluence_search no longer attributed to _resolve_space_key_via_list.

The new docstring lists confluence_search under the primary-route group. Confirmed at gateway/gateway.py:5700-5701: confluence_search catches ConfluenceUpstreamForbidden directly from client.search_cql(...) and translates to confluence_upstream_403. It never invokes _resolve_space_key_via_list. Fixed.

2. All eight primary routes enumerated; three auxiliary sites enumerated.

Cross-checked against the except ConfluenceUpstreamForbidden handlers in gateway/gateway.py:

Primary route	Catch site	Event
confluence_page_get	5088-5089	confluence_upstream_403
confluence_page_descendants (primary fetch)	5190-5191	confluence_upstream_403
confluence_page_footer_comments (primary fetch)	5299-5300	confluence_upstream_403
confluence_page_inline_comments (primary fetch)	5408-5409	confluence_upstream_403
confluence_space_pages	5524-5527, 5550-5552	confluence_upstream_403
confluence_space_list	5606-5607	confluence_upstream_403
confluence_search	5700-5701	confluence_upstream_403
confluence_execute (primary execute_raw)	5829-5830	confluence_upstream_403

Auxiliary site	Catch site	Effective event
_resolve_space_key_via_list (via _check_post_fetch_space_allowlist)	4974-4985	route's confluence_*_denied, space_key=None
Parent re-fetch in descendants / footer-comments / inline-comments	5214-5237, 5320-5349, 5430-5458	confluence_space_denied, space_key=None
confluence_execute cache-warm in space_id_in_path branch	5871-5888	confluence_execute_denied, space_key=resolved (None)

All match the new docstring.

3. Module-level docstring no longer claims uniform translation.

gateway/confluence_client.py:62-67 now points readers at the ConfluenceUpstreamForbidden class docstring for the per-call-site taxonomy rather than repeating the inaccurate "all read methods … audit it as confluence_upstream_403" framing. Fixed.

Other checks

• Diff scope (git diff a7778f2..9eed744 --stat): one file, +18/−8, docstring-only.
• No code, test, or config changes — behavior identical to a7778f2.
• No security / correctness / robustness implications.

Nit (non-blocking)

The commit title says "match all nine routes" but the docstring (and commit body) describe eight primary routes plus three auxiliary call sites. Pure title typo — the docstring content is accurate. Not worth a follow-up.

Pre-merge obligation reminder

The conditional-ACK obligation from earlier in the cycle still applies and is unaffected by this delta: the human merger must resolve shared/egg_restrictions/patterns.py against main's 2f693f3e / PR #2135 (drop the "sandbox/scripts/" block entry plus both block_exempt_patterns exemptions for jira and confluence). Already in the PR body's "⚠️ Pre-merge Obligations" section.

Verdict

Comment / approve. The docstring now accurately matches the code. Self-authored PR, so using --comment.

— Authored by egg

[james-in-a-box]

Automated feedback loop has reached the maximum of 5 rounds. Human review is needed to make further progress on this PR.

[james-in-a-box]

egg review completed. View run logs

26 previous review(s) hidden.