Skip to content

refactor(providers/pi): drop rot-prone file:line refs from Pi comments#1275

Merged
Wirasm merged 1 commit intodevfrom
fix/pi-comment-path-rot
Apr 17, 2026
Merged

refactor(providers/pi): drop rot-prone file:line refs from Pi comments#1275
Wirasm merged 1 commit intodevfrom
fix/pi-comment-path-rot

Conversation

@Wirasm
Copy link
Copy Markdown
Collaborator

@Wirasm Wirasm commented Apr 17, 2026

Summary

Follow-up to #1270. The multi-agent review on #1271 flagged the CLAUDE.md comment rule ("don't embed paths/callers that rot as the codebase evolves"). The rule applies equally to three spots in the just-merged Pi provider that embed packages/.../provider.ts:N-M line ranges.

Changes

Three inline comments in `packages/providers/src/community/pi/`:

  • `session-resolver.ts` — ResolvedSession's `resumeFailed` JSDoc
  • `provider.ts` — Claude auth-merge reference (line ~122)
  • `provider.ts` — Codex fallback reference (line ~204)
  • `provider.ts` — upstream Pi `auth-storage.ts:424-485` ref

Each drops the fragile line numbers and file paths while keeping the conceptual cross-reference ("mirrors Claude's process-env + request-env merge pattern", "matches the Codex provider's fallback pattern", etc.) — that's the load-bearing part of the comment.

Why

Line numbers drift the moment the referenced files change. The Claude and Codex providers are both under active evolution; the Pi provider doesn't benefit from pinning line ranges at a specific snapshot.

Test plan

  • `bun run validate` passes (no behavior change; comment-only refactor).

Summary by CodeRabbit

  • Documentation
    • Updated internal documentation comments for the Pi provider to clarify environment variable merge patterns and credential resolution behavior.

Applies the CLAUDE.md comment rule ("don't embed paths/callers that rot
as the codebase evolves") flagged by the PR #1271 review to the Pi
provider's inline comments.

Three spots in the merged Pi code embed `packages/.../provider.ts:N-M`
line ranges pointing at the Claude and Codex providers. These ranges
will drift the moment those files change — the Claude auth-merge
pattern's line numbers are already off-by-a-few in some local branches.

Keep the conceptual cross-reference ("mirrors Claude's process-env +
request-env merge pattern", "matches the Codex provider's fallback
pattern for the same condition") — that's the load-bearing part of the
comment — drop the fragile line numbers and file paths.

Same treatment for the upstream Pi auth-storage.ts:424-485 reference,
which points at a specific line range in a moving dependency.

No behavior change; comment-only refactor.
@Wirasm Wirasm merged commit c864d8e into dev Apr 17, 2026
2 of 3 checks passed
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Apr 17, 2026

Caution

Review failed

The pull request is closed.

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 31179689-248c-4400-b584-034dd6d640fb

📥 Commits

Reviewing files that changed from the base of the PR and between 4e56991 and 7d61dd3.

📒 Files selected for processing (2)
  • packages/providers/src/community/pi/provider.ts
  • packages/providers/src/community/pi/session-resolver.ts

📝 Walkthrough

Walkthrough

These changes update inline documentation and JSDoc comments in the Pi provider module to generalize references to environment-variable merge patterns and fallback behavior, removing explicit file/line references. No runtime logic, control flow, or public APIs were modified.

Changes

Cohort / File(s) Summary
Pi Provider Documentation Updates
packages/providers/src/community/pi/provider.ts, packages/providers/src/community/pi/session-resolver.ts
Revised inline documentation to replace specific file/line references with generalized descriptions of environment-variable merge patterns and fallback behavior (e.g., "process-env + request-env merge pattern" and "resume_thread_failed fallback pattern").

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~2 minutes

Poem

🐰 Our docs grew cleaner, more refined,
No line numbers cluttering the mind,
Pi's patterns now described with grace,
General wisdom in every place! ✨

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/pi-comment-path-rot

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@Wirasm Wirasm deleted the fix/pi-comment-path-rot branch April 17, 2026 12:08
ztech-gthb pushed a commit to ztech-gthb/Archon that referenced this pull request Apr 18, 2026
…nts (coleam00#1275)

Applies the CLAUDE.md comment rule ("don't embed paths/callers that rot
as the codebase evolves") flagged by the PR coleam00#1271 review to the Pi
provider's inline comments.

Three spots in the merged Pi code embed `packages/.../provider.ts:N-M`
line ranges pointing at the Claude and Codex providers. These ranges
will drift the moment those files change — the Claude auth-merge
pattern's line numbers are already off-by-a-few in some local branches.

Keep the conceptual cross-reference ("mirrors Claude's process-env +
request-env merge pattern", "matches the Codex provider's fallback
pattern for the same condition") — that's the load-bearing part of the
comment — drop the fragile line numbers and file paths.

Same treatment for the upstream Pi auth-storage.ts:424-485 reference,
which points at a specific line range in a moving dependency.

No behavior change; comment-only refactor.
joaobmonteiro pushed a commit to joaobmonteiro/Archon that referenced this pull request Apr 26, 2026
…nts (coleam00#1275)

Applies the CLAUDE.md comment rule ("don't embed paths/callers that rot
as the codebase evolves") flagged by the PR coleam00#1271 review to the Pi
provider's inline comments.

Three spots in the merged Pi code embed `packages/.../provider.ts:N-M`
line ranges pointing at the Claude and Codex providers. These ranges
will drift the moment those files change — the Claude auth-merge
pattern's line numbers are already off-by-a-few in some local branches.

Keep the conceptual cross-reference ("mirrors Claude's process-env +
request-env merge pattern", "matches the Codex provider's fallback
pattern for the same condition") — that's the load-bearing part of the
comment — drop the fragile line numbers and file paths.

Same treatment for the upstream Pi auth-storage.ts:424-485 reference,
which points at a specific line range in a moving dependency.

No behavior change; comment-only refactor.
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.

1 participant