Skip to content

feat: add protected Pi Confluence tools - #225

Open
arc-terry wants to merge 23 commits into
pchuri:mainfrom
arc-terry:feature/pi-confluence-tools
Open

feat: add protected Pi Confluence tools#225
arc-terry wants to merge 23 commits into
pchuri:mainfrom
arc-terry:feature/pi-confluence-tools

Conversation

@arc-terry

Copy link
Copy Markdown

Summary

Add a typed, protected Confluence integration for the Pi Coding Agent.

Changes

  • Add 13 typed Confluence read tools.
  • Add protected Confluence mutation tools with:
    • Explicit write opt-in.
    • Space allowlisting.
    • Interactive Pi confirmation.
    • Exact confirmation phrases for destructive operations.
    • Canonical page title, ID, and space validation.
  • Add mandatory previews and one-use approvals for bulk copy and version purge operations.
  • Add package-local, shell-free CLI execution with bounded output, timeouts, abort handling, and credential redaction.
  • Add payload limits and project-contained file validation.
  • Add preflight ownership and target verification.
  • Add comprehensive tests and update Pi/Confluence documentation.

Security

  • Writes are disabled by default.
  • Generic API, arbitrary argv, shell commands, and model-controlled HTTP methods are not exposed.
  • Automated tests do not perform authenticated Confluence mutations.
  • Confluence responses are treated as untrusted external content.

Testing

  • npm test -- --runInBand — 32 suites passed, 1,060 tests passed.
  • npm run lint — passed.

Notes

The feature branch contains the sanitized history and excludes Superpowers artifacts.

@arc-terry
arc-terry marked this pull request as ready for review August 23, 2026 13:02

@pchuri pchuri left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for this contribution — the scale and care here are impressive. The protection design (write opt-in, space allowlisting, one-use preview approvals, exact confirmation phrases, project-path containment) is genuinely well thought out, all 1,060 tests pass locally, and I found no classic path-traversal / shell-injection / prefix-match issues. The copy-tree dry-run filter change is also a correct fix — it aligns the dry-run count with what copyPageTree actually creates.

Before I can merge, a few critical issues need fixing. I'm deliberately limiting this round to blockers only:

1. Packaging: mandatory peer deps reach every npm user (package.json)
@earendil-works/pi-coding-agent and typebox are non-optional peers, so npm ≥7 auto-installs them for every npm install -g confluence-cli. The lockfile grows 409 → 551 packages (AWS SDK, Anthropic SDK, Google GenAI, native binaries), and scripts/generate-prod-shrinkwrap.sh filters only dev: true, so the published shrinkwrap would pin all of it as production deps. The test suite passes with only typebox present, so the agent peer is type-only. Please mark both peers optional via peerDependenciesMeta, and exclude peer: true entries in the shrinkwrap script.

2. Credentials can reach model context (lib/pi/command-runner.js redactText; lib/confluence-client.js:380)
redactText only masks values present in env — with credentials in ~/.confluence-cli/config.json or ~/.netrc (HOME is forwarded) it's a no-op. And the display-URL error path console.errors the full AxiosError (~11KB), which includes the Authorization header; for basic auth it's base64, so even env-based substring redaction misses it. Reproduced against axios 1.18.1. Please stop dumping the raw AxiosError and redact Authorization-header forms.

3. Read tools can overwrite project files with writes disabled (.pi/extensions/confluence-cli.ts normalizeReadInput; bin/commands/export.js:251)
confluence_convert is registered unconditionally (even under CONFLUENCE_READ_ONLY), and resolveProjectOutputPath returns an existing regular file unchecked — confluence_convert {outputFile: "package.json"} silently replaces it (.git/hooks is reachable too). Separately, export --dry-run is honored only in the recursive branch, so a non-recursive dryRun: true still writes files. Please refuse to overwrite existing files from read tools (or gate them behind the write opt-in), and fix the non-recursive dry-run.

4. The fixed 48KB output cap makes core tools structurally unusable at real-world scale (lib/pi/preflight.js)
Measured with the CLI's pretty-printed JSON: confluence_create fails permanently on instances with ~500+ spaces (spaces --all enumeration), copy_tree_preview/copy_tree break at ~328 descendants, and comment_delete/attachment_delete are blocked on heavy pages (25 comments × ~400-char bodies; ~141 attachments). Also comment_delete of a reply always fails because the ownership scan never passes --depth all. Targeted lookups (single-space check instead of full enumeration, threading depth) and per-operation output limits would resolve these.

5. Space keys are force-uppercased into outbound mutations (lib/pi/preflight.js requireSpaceKey / handleCreate)
handleCreate writes the uppercased key back into input.spaceKey, so on Server/DC a space with real key dev or ~jsmith gets DEV/~JSMITH forwarded to create — wrong space or not-found. Case-folding is fine for allowlist matching, but the original casing must be preserved in the executed args.

I have a longer list of non-blocking findings (diagnosability, efficiency, contract-test coverage); happy to file those as follow-up issues so this PR stays focused. Thanks again — this is a strong foundation.

@arc-terry

Copy link
Copy Markdown
Author

Hi Pchuri,

My original goal was to make the repository directly installable through the Pi agent command:
pi install "git@github.com:pchuri/confluence-cli.git"
That was the main motivation behind these changes. I’ll work on addressing the five blocking issues you identified in the near future.
Thank you for the quick and detailed review.

@pchuri

pchuri commented Aug 25, 2026

Copy link
Copy Markdown
Owner

Thanks for the context — making the repo directly installable via pi install "git@github.com:pchuri/confluence-cli.git" is a use case I'm happy to support.

One reassurance on blocker #1: marking the peers optional won't break your goal. The pi install flow (clone + npm ci) installs from the lockfile regardless of peer optionality, so the local-checkout path keeps working — the change only protects ordinary npm install -g confluence-cli users from pulling the Pi agent tree. When you update, please also regenerate package-lock.json so the Pi dependency tree drops out of it, and add a peer: true filter to scripts/generate-prod-shrinkwrap.sh.

No rush — ping me when it's ready and I'll re-review promptly.

@arc-terry

Copy link
Copy Markdown
Author

Blocker #1 is ready in c18b17f.

Installation behavior

User / command Before After
Normal npm user: npm install -g confluence-cli npm auto-installed @earendil-works/pi-coding-agent, typebox, and the large Pi dependency tree. Installs only Confluence CLI production dependencies; no Pi coding agent or TypeBox.
Pi local-Git user: pi install "git@github.com:pchuri/confluence-cli.git" Clone + npm ci installed the larger 550-package lockfile. Clone + npm ci still loads the Pi extension; lockfile is 410 packages. typebox is available as a dev dependency for the extension tests, while Pi supplies its own host runtime.

Changes

  • Removed @earendil-works/pi-coding-agent as a package peer: the extension imports it only as a type, and Pi already provides the host runtime.
  • Kept typebox as the sole optional runtime peer.
  • Regenerated package-lock.json, removing the Pi dependency tree (550 → 410 packages).
  • Updated scripts/generate-prod-shrinkwrap.sh to exclude peer:true entries.
  • Added regression coverage for optional peer metadata and shrinkwrap filtering.

Verification: clean npm ci plus Pi-extension tests passed; a packed normal-user install contains neither Pi coding agent nor TypeBox.

@pchuri

pchuri commented Aug 26, 2026

Copy link
Copy Markdown
Owner

Thanks — the packaging fix looks good, and dropping the pi-coding-agent peer entirely (it's type-only) while adding typebox as a devDependency is a better solution than what I suggested: the test suite is now self-contained. Previously I had to vendor typebox by hand to run it; now a plain npm ci gives 33 suites / 1,063 tests passing.

I verified the three install paths end to end:

  • package-lock.json: 551 → 411 packages (main is 409; the +2 are typebox and jiti, both dev), with zero AWS / Anthropic / GenAI entries
  • scripts/generate-prod-shrinkwrap.sh: 100 production packages, typebox and jiti both excluded, root devDependencies cleared
  • npm pack + install into a clean project: 99 packages, no typebox, no @earendil-works

The regression test in tests/prod-shrinkwrap.test.js is a nice touch.

Two small non-blocking notes for whenever you next touch this:

  1. "typebox": "*" in devDependencies is looser than the rest of this repo's dependency policy (we pin transitives via overrides). The lockfile and npm ci protect CI, but a contributor running plain npm install would pick up a future 2.x major. ^1.3.18 would be safer.
  2. .pi/ and the pi manifest field are still published, but typebox is an optional peer and therefore won't be installed for npm consumers — so loading the extension from an npm-installed copy would fail on a missing typebox. The documented local-checkout path (npm cipi install <path>) is unaffected; a one-line note in the README would save someone the confusion.

No rush on the remaining four blockers — take the time you need, and ping me when they're ready.

@arc-terry

Copy link
Copy Markdown
Author

I'm glad to see your comment.
My plan is to address the major blockers first. Blocker 2 is already in progress.
I'd prefer to keep the non-blocking notes as items for future follow-up.

Thanks!

@arc-terry

Copy link
Copy Markdown
Author

Blocker #2 is ready in 9b3da95.

Goal

Prevent credentials from config files, .netrc, Axios errors, and nested command output from reaching Pi/model context.

Credential handling

Situation Before After
Config or .netrc credentials Environment-only redaction could miss credentials not present in environment variables. Authorization and Cookie header forms are redacted independently of environment values.
Basic authentication Raw Axios errors could include base64-encoded Basic credentials. Basic Authorization values are replaced with [REDACTED].
Display-URL lookup failure The CLI logged the complete AxiosError, including request configuration and headers. Logs only a safe status, such as Error resolving page ID from display URL (HTTP 401).
Parsed JSON tool output Authorization or Cookie fields could remain in Pi-facing JSON. Sensitive fields are structurally replaced with [REDACTED].
Nested serialized JSON Credentials hidden inside multiple JSON-string layers could evade text redaction. Credential fields are redacted through four nested JSON layers; rebuilt stdout and JSON contain no secret.

Files changed

File Purpose
lib/confluence-client.js Replaces raw display-URL Axios-error logging with a safe HTTP-status diagnostic.
lib/pi/command-runner.js Adds Authorization/Cookie header redaction, structured JSON redaction, and bounded nested JSON redaction.
tests/confluence-client.test.js Verifies that display-URL failures do not log Basic credentials or raw Axios errors.
tests/display-url-errors.test.js Verifies safe human-readable and structured JSON error behavior.
tests/pi-command-runner.test.js Covers config-style credentials, serialized headers, multi-cookie values, parsed JSON, and four nested JSON layers.

Changes

  • Removed raw Axios-error logging from display-URL failures.
  • Added safe HTTP-status diagnostics.
  • Redacted Basic/Bearer Authorization and Cookie values without relying on environment credentials.
  • Added structural and bounded nested JSON credential redaction.

Verification: independent TDD security review passed; 33 suites and 1,069 tests passed; lint passed.

@arc-terry

Copy link
Copy Markdown
Author

Blocker #3 is ready in 66ee0c0.

Goal

Prevent confluence_convert from overwriting project files or writing into Git metadata, and ensure non-recursive export dry-run never creates or changes files.

Read-tool file safety

Situation Before After
CONFLUENCE_READ_ONLY=true Blocked Confluence server mutations, but confluence_convert remained registered as a read tool and could overwrite local project files. Convert remains available for safe local use, but it can create only a new file, cannot replace an existing file, and cannot write inside .git.
confluence_convert { outputFile: "package.json" } Existing regular files were accepted and silently replaced. Existing output files are rejected with PROJECT_PATH before the CLI command runs.
Convert to a new project file Allowed. Still allowed when the path is inside the project and does not already exist.
Convert to .git/hooks/pre-commit A new Git hook path was reachable in a normal checkout. Output paths inside .git are rejected before command execution.
Symlink or traversal output Could potentially escape without canonical path validation. Existing canonical-path and symlink protections remain enforced.
Non-recursive export --dry-run Read page content, created directories/files and markers, and could download attachments. Fetches only page metadata, prints the planned content path, and returns without filesystem changes or downloads.
Recursive export --dry-run Already avoided writes. Existing behavior remains unchanged.

CONFLUENCE_READ_ONLY continues to protect against remote Confluence mutations. Local read-tool outputs now have separate filesystem protections, so convert cannot bypass read-only safety by replacing project files.

Files changed

File Purpose
.pi/extensions/confluence-cli.ts Applies protected read-output validation and create-only validation to convert output files.
lib/pi/write-authorization.js Adds .git metadata protection and rejects existing regular output files.
bin/commands/export.js Returns early for non-recursive dry-run before content reads, writes, markers, or attachment operations.
tests/pi-extension-tools.test.js Covers existing-file rejection, .git/hooks rejection, and safe new output paths.
tests/export.test.js Covers non-recursive dry-run with no content read, download, or filesystem artifacts.

Changes

  • Refused existing convert output files before command execution.
  • Blocked convert output paths inside project .git metadata.
  • Preserved safe new project outputs and existing escape protections.
  • Fixed non-recursive export dry-run to perform preview-only behavior.

Verification: three phase-gated TDD cycles and independent security reviews passed; 33 suites and 1,072 tests passed; lint passed.

@arc-terry

Copy link
Copy Markdown
Author

Blocker #4 is ready in c6995fe.

Goal

Make Pi preflight and core tools usable on large Confluence instances without removing output safety limits.

Large-instance behavior

Original problem Before After
Fixed 48 KiB cap Every operation used the same limit regardless of response shape. Operations receive bounded limits from 16 KiB to 1 MiB according to their output requirements.
confluence_create with ~500+ spaces Preflight called spaces --all; pretty-printed JSON exceeded 48 KiB and permanently blocked creation. Performs one direct space lookup by key using a 16 KiB budget.
copy_tree_preview with ~328+ descendants Returned the complete plannedTree, exceeding 48 KiB. Returns identities, versions, count, root title, and a compact SHA-256 fingerprint.
copy_tree at scale Approval depended on the oversized preview and execution shared the 48 KiB limit. Preview fits within 32 KiB; copy execution receives a bounded 1 MiB budget.
comment_delete on heavy pages Preflight listed comments and included expanded bodies while searching for one ID. Looks up only the requested comment and returns no body content.
Reply-comment deletion Ownership scan did not pass --depth all, so replies could never be found. Direct comment lookup verifies the reply’s page container without enumerating threads.
attachment_delete with ~141+ attachments Preflight enumerated every attachment and could overflow before finding the target. Looks up only the requested attachment and verifies its page container.
Target beyond the first page Ownership depended on pagination position and limits. Direct ID lookup is independent of pagination.
Changed copy plan Full plans were too large to compare reliably. Fresh count, version, identity, title, and fingerprint facts are compared before mutation.
Missing target Direct lookup failures could become generic CLI failures. Compact missing results map to TARGET_NOT_FOUND.
Wrong destructive target type A matching content ID could be interpreted as the wrong resource type. Comment and attachment lookups validate their Confluence content type.

Output budgets

Budget Operations
16 KiB Targeted space, comment, and attachment lookups
32 KiB Compact copy-tree preview
256 KiB Spaces, comments, attachments, search, children, versions, and other list operations
1 MiB Content reads, conversion, property reads, and copy execution
48 KiB Unspecified operations

No operation is unlimited. Existing OUTPUT_TRUNCATED and UNKNOWN_RESULT behavior remains unchanged.

Files changed

File Purpose
.pi/extensions/confluence-cli.ts Uses operation-specific budgets and preserves compact approval facts during stale-plan checks.
lib/pi/operation-policy.js Defines targeted internal operations and per-operation output budgets.
lib/pi/preflight.js Replaces broad enumeration with targeted lookups and validates compact copy fingerprints.
lib/pi/copy-plan.js Generates deterministic canonical SHA-256 plan fingerprints.
lib/confluence-client.js Adds direct space/comment/attachment metadata lookups with type and ownership normalization.
bin/confluence.js Adds direct space lookup and compact copy-tree preview output.
bin/commands/comment.js Adds compact comment-by-ID output without bodies.
bin/commands/attachments.js Adds compact attachment-by-ID output without download URLs.
tests/* Covers large fixtures, replies, ownership mismatches, missing records, budgets, compact plans, and stale-plan rejection.

Changes

  • Replaced spaces --all create preflight with one targeted lookup.
  • Replaced comment and attachment list scans with direct ID ownership checks.
  • Added reply-comment support without full-thread enumeration.
  • Replaced full copy approval payloads with deterministic fingerprints.
  • Added bounded per-operation output budgets.
  • Added safe not-found, malformed-result, ownership, and content-type validation.

Verification: phase-gated TDD and task-scoped reviews completed; final fix-wave re-review approved; 34 suites and 1,114 tests passed; lint passed.

@arc-terry

Copy link
Copy Markdown
Author

Blocker #5 is ready in 1c8656d.

Goal

Preserve real Confluence Server/Data Center space-key casing in outbound mutations while keeping authorization checks case-insensitive.

Space-key behavior

Situation Before After
Create in lowercase space dev Preflight changed it to DEV before lookup and execution. Sends the trimmed requested key to lookup and uses the server-returned canonical key unchanged.
Personal space ~jsmith Preflight changed it to ~JSMITH, potentially targeting the wrong or nonexistent space. Preserves ~jsmith through lookup, approval, and final create argv.
Server returns canonical casing Returned keys were force-uppercased by requireSpaceKey(). Server-returned casing is preserved in targets, summaries, and mutation input.
Requested key uses different letter case Matching depended on the uppercased mutation value. Requested and returned keys are compared case-insensitively, but execution uses the server’s canonical spelling.
Lookup returns a different space Could be accepted after normalization in ambiguous cases. Case-folded mismatch is rejected before approval or mutation.
Write allowlist Keys were uppercased for comparison. Remains case-insensitive: an allowlist entry such as ~ALICE permits canonical key ~alice.
Page target summaries Server-returned page space keys were uppercased. Summaries preserve the server’s actual space-key casing.

Files changed

File Purpose
lib/pi/preflight.js Separates casing-preserving key validation from case-insensitive identity comparison and uses canonical server keys for create execution.
tests/pi-preflight.test.js Covers lowercase, personal-space, mixed-case, mismatch, and server-canonical casing behavior.
tests/pi-extension-tools.test.js Verifies final create CLI argv preserves canonical casing.
tests/pi-write-authorization.test.js Verifies allowlist matching remains case-insensitive.

Changes

  • Removed forced uppercasing from preflight space-key normalization.
  • Preserved trimmed requested keys during targeted lookup.
  • Added case-insensitive requested/server identity verification.
  • Used server-returned canonical keys in approval targets and mutation arguments.
  • Retained uppercase folding only inside allowlist authorization comparison.

Verification: phase-gated TDD and independent review completed; 34 suites and 1,119 tests passed; lint passed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants