Skip to content

fix: harden archive and xurl portability#79

Open
rodriguez46p-ui wants to merge 4 commits into
steipete:mainfrom
rodriguez46p-ui:loop/birdclaw-archive-xurl-fixes-v2
Open

fix: harden archive and xurl portability#79
rodriguez46p-ui wants to merge 4 commits into
steipete:mainfrom
rodriguez46p-ui:loop/birdclaw-archive-xurl-fixes-v2

Conversation

@rodriguez46p-ui

Copy link
Copy Markdown

Summary

  • Normalize archive discovery and LaunchAgent paths to POSIX-style separators for stable Windows/macOS behavior.
  • Make archive import tests zip fixtures with fflate instead of relying on a system zip binary.
  • Harden backup git sync by writing LF .gitattributes, requiring the configured backup path to be the git toplevel, and fetching origin/main into the remote-tracking ref.
  • Add optional direct X API bearer-token transport with test isolation via BIRDCLAW_DISABLE_BEARER_TRANSPORT=1.

Test Plan

  • npx --yes node@25.8.1 ./node_modules/vitest/vitest.mjs run src/lib/xurl.test.ts src/lib/archive-import.test.ts src/lib/backup.test.ts src/lib/launchd.test.ts
  • corepack pnpm exec oxfmt --check src/lib/archive-finder.ts src/lib/archive-import.test.ts src/lib/backup.ts src/lib/launchd.ts src/lib/xurl.test.ts src/lib/xurl.ts src/test/setup.ts
  • corepack pnpm exec oxlint src/lib/archive-finder.ts src/lib/archive-import.test.ts src/lib/backup.ts src/lib/launchd.ts src/lib/xurl.test.ts src/lib/xurl.ts src/test/setup.ts
  • corepack pnpm exec tsgo --noEmit
  • git diff --check

Note: local oxlint exits 0 but reports one existing eslint(no-control-regex) warning on the pre-existing ANSI stripping regex in src/lib/xurl.ts.

@clawsweeper

clawsweeper Bot commented Jun 27, 2026

Copy link
Copy Markdown

Codex review: needs real behavior proof before merge. Reviewed June 28, 2026, 12:15 AM ET / 04:15 UTC.

Summary
The PR changes package metadata, API contracts, archive finder/import tests, backup Git setup, LaunchAgent plist generation, profile hydration, and xurl transport code to add portability hardening and optional bearer-token reads.

Reproducibility: yes. from source inspection: set a bearer-token env var in a setup that also has xurl OAuth available, and the PR head returns availableTransport: "bearer" while authored sync and live data-source status still require xurl. I did not run a live X setup in this read-only review.

Review metrics: 3 noteworthy metrics.

  • Changed files: 15 files, +414/-22. The diff crosses auth transport, backup behavior, archive tests, LaunchAgent output, package metadata, and API contracts, so single-path review is not enough.
  • Dependency surface: 1 dev dependency added. fflate is pinned in package metadata and lockfile for test zip fixture generation, which matters for supply-chain review.
  • Transport surface: 1 status value and 5 env vars added. The new bearer status and bearer-control env vars affect provider routing, tests, and existing xurl status consumers.

Merge readiness
Overall: 🧂 unranked krab
Proof: 🧂 unranked krab
Patch quality: 🧂 unranked krab
Result: blocked until real behavior proof from a real setup is added.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Rank-up moves:

  • [P1] Add redacted real behavior proof for the changed archive discovery/import, LaunchAgent plist generation, backup sync, and bearer/xurl paths; redact tokens, endpoints, IPs, phone numbers, and other private details.
  • Preserve existing xurl-authenticated workflows when bearer env vars are present, with focused tests for authored sync and live data-source status.
  • [P1] Get maintainer acceptance or documentation for the nested-worktree backup fail-closed upgrade behavior.

Proof guidance:

  • [P1] Needs real behavior proof before merge: Only tests/check output is present; add redacted real setup logs, terminal output, screenshots, recordings, or artifacts for the archive, LaunchAgent, backup, and bearer/xurl paths before merge, then update the PR body for re-review.

Risk before merge

  • [P1] Bearer-token env vars can make Birdclaw report availableTransport: "bearer" before probing xurl, which can break authored sync and misreport live data sources in environments that already have xurl OAuth configured.
  • [P1] The nested-worktree backup guard intentionally changes first-sync behavior for configured backup paths under a non-home parent Git worktree; maintainers need to accept or document that upgrade behavior before merge.
  • [P1] Real behavior proof is still absent for the changed archive, LaunchAgent, backup, and bearer/xurl runtime paths; tests and CI-style output do not prove the contributor's real setup.

Maintainer options:

  1. Preserve xurl status compatibility before merge (recommended)
    Update the status model or downstream guards so bearer env vars do not hide an authenticated xurl setup, and cover authored sync plus live data-source status in tests.
  2. Accept and document the backup fail-closed change
    Maintainers can keep the nested-worktree guard if they explicitly accept the first-sync breakage and document the migration path for affected backup.repoPath users.
  3. Split bearer transport if direction is not ready
    If the bearer status/product surface is still uncertain, land the narrower portability and backup pieces separately and move bearer support to a focused follow-up.

Next step before merge

  • [P1] Manual review is needed because contributor proof is mock-only and maintainers need to choose the bearer status and backup upgrade behavior before merge.

Security
Cleared: No concrete security or supply-chain regression was found; bearer tokens are sent only as Authorization headers and fflate is a pinned dev dependency with lockfile integrity.

Review findings

  • [P1] Preserve xurl availability when bearer tokens are set — src/lib/xurl.ts:229-235
  • [P2] Add an upgrade path for nested-worktree backup failures — src/lib/backup.ts:617-621
Review details

Best possible solution:

Preserve xurl availability independently of bearer support, add coverage for authored/data-source status consumers, and land the backup fail-closed behavior only with maintainer-accepted upgrade docs or proof.

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

Yes, from source inspection: set a bearer-token env var in a setup that also has xurl OAuth available, and the PR head returns availableTransport: "bearer" while authored sync and live data-source status still require xurl. I did not run a live X setup in this read-only review.

Is this the best way to solve the issue?

No. The portability/test hardening is useful, but the safest path is to preserve xurl availability separately from bearer availability and handle the backup upgrade behavior explicitly before merge.

Full review comments:

  • [P1] Preserve xurl availability when bearer tokens are set — src/lib/xurl.ts:229-235
    This returns availableTransport: "bearer" before probing xurl whenever a bearer env var is present. Existing authored sync still rejects non-xurl status, and live data-source status still marks only xurl as working, so users with xurl OAuth plus X_BEARER_TOKEN or TWITTER_BEARER_TOKEN can lose working xurl flows. Preserve xurl availability separately or update the affected guards and tests before merging.
    Confidence: 0.88
  • [P2] Add an upgrade path for nested-worktree backup failures — src/lib/backup.ts:617-621
    The new guard runs before clone/init, so a configured backup path inside a non-home parent Git worktree that Birdclaw would previously initialize now fails at runtime. Add maintainer-accepted docs, migration behavior, or an explicit opt-in/compatibility path so existing first-sync setups do not fail without warning.
    Confidence: 0.74

Overall correctness: patch is incorrect
Overall confidence: 0.86

AGENTS.md: not found in the target repository.

Codex review notes: model internal, reasoning high; reviewed against 10f98d3fb36a.

Label changes

Label justifications:

  • P2: This is a normal-priority portability/auth/backup improvement with limited blast radius but merge-blocking compatibility and proof gaps.
  • merge-risk: 🚨 auth-provider: The PR adds bearer-token auth routing and changes the transport status model used by existing xurl OAuth flows.
  • merge-risk: 🚨 compatibility: The PR can break existing xurl-gated behavior and changes first-sync backup initialization under parent Git worktrees.
  • rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🧂 unranked krab and patch quality is 🧂 unranked krab.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: Only tests/check output is present; add redacted real setup logs, terminal output, screenshots, recordings, or artifacts for the archive, LaunchAgent, backup, and bearer/xurl paths before merge, then update the PR body for re-review.
Evidence reviewed

What I checked:

  • Target AGENTS.md absent: No AGENTS.md exists inside the Birdclaw target repository; the parent ClawSweeper AGENTS.md was read but not applied as target repository policy. (10f98d3fb36a)
  • PR surface: GitHub reports 15 changed files with 414 additions and 22 deletions, touching auth transport, backup behavior, archive tests, LaunchAgent output, package metadata, and API contracts. (a2d136c27457)
  • Bearer status masks xurl probing: On the PR head, readTransportStatusEffect returns availableTransport: "bearer" before checking whether xurl is installed or authenticated. (src/lib/xurl.ts:229, a2d136c27457)
  • Authored sync still requires xurl status: On the same PR head, authored sync fails when availableTransport is anything other than "xurl", so a bearer-token environment can block an otherwise valid xurl OAuth flow. (src/lib/authored-live.ts:398, a2d136c27457)
  • Data-source status still treats only xurl as working: The live data-source status view still computes works only from availableTransport === "xurl", so bearer status is surfaced as warning/non-working despite adding bearer reads. (src/lib/data-sources.ts:139, a2d136c27457)
  • API contract updated for bearer: The PR head updates queryEnvelopeSchema to accept availableTransport: "bearer" and adds a contract test, so the previous runtime schema drift is addressed. (src/lib/api-contracts.ts:254, a2d136c27457)

Likely related people:

  • steipete: History and blame point to Peter Steinberger for the xurl status, live data-source, backup, launchd, and release history around this PR. (role: primary area contributor; confidence: high; commits: b456320a66aa, f0e4de860c1a, fa966422f256; files: src/lib/xurl.ts, src/lib/data-sources.ts, src/lib/backup.ts)
  • cavit99: Cavit Erginsoy introduced the authored sync path that still gates on availableTransport === "xurl" and is affected by the new bearer status. (role: affected feature contributor; confidence: medium; commits: 21faa1d17ec1; files: src/lib/authored-live.ts, src/lib/xurl.ts)
  • uwe-schwarz: Uwe authored recent JSONL backup line-separator work adjacent to this PR's LF and .gitattributes backup hardening. (role: adjacent backup contributor; confidence: medium; commits: b30b21808619, 7622e77; files: src/lib/backup.ts, src/lib/backup.test.ts)
  • kyupark: Kyu Park previously repaired xurl unauthenticated status detection, which is the same status surface changed by the bearer transport work. (role: xurl status contributor; confidence: medium; commits: 1457e4772687; files: src/lib/xurl.ts, src/lib/xurl.test.ts)
What the crustacean ranks mean
  • 🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
  • 🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
  • 🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
  • 🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
  • 🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
  • 🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
  • 🌊 off-meta tidepool: rating does not apply to this item.

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

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

@clawsweeper clawsweeper Bot added rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. P2 Normal priority bug or improvement with limited blast radius. merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. merge-risk: 🚨 auth-provider 🚨 Merging this PR could break OAuth, tokens, provider routing, model choice, or credentials. labels Jun 27, 2026
@rodriguez46p-ui

Copy link
Copy Markdown
Author

Addressed the xurl/bearer review findings in f72d6e1.

What changed:

  • OAuth2-selected xurl calls now stay on xurl even when BIRDCLAW_X_BEARER_TOKEN, X_BEARER_TOKEN, or TWITTER_BEARER_TOKEN are set.
  • Direct bearer fetches now receive the existing JSON command options: AbortSignal, timeout budget, retry budget, and onAttempt telemetry.
  • Added regression coverage for both cases.

Verification from this branch:

  • npx --yes node@25.8.1 ./node_modules/vitest/vitest.mjs run src/lib/xurl.test.ts src/lib/archive-import.test.ts src/lib/backup.test.ts src/lib/launchd.test.ts — passed, 4 files / 133 tests.
  • corepack pnpm exec tsgo --noEmit — passed.
  • corepack pnpm exec oxfmt --check src/lib/xurl.ts src/lib/xurl.test.ts — passed.
  • corepack pnpm exec oxlint src/lib/xurl.ts src/lib/xurl.test.ts src/lib/archive-import.test.ts src/lib/archive-finder.ts src/lib/backup.ts src/lib/launchd.ts src/lib/backup.test.ts src/lib/launchd.test.ts — exit 0; still reports the pre-existing no-control-regex warning in src/lib/xurl.ts.
  • git diff --check — passed, with Windows LF→CRLF working-copy warnings only.
  • GitGuardian Security Checks — passed on f72d6e1.

I still cannot provide live X API/LaunchAgent proof from this cron host without maintainer credentials/runtime setup, but the auth-routing and option-forwarding blockers are now covered by focused tests.

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented Jun 27, 2026

Copy link
Copy Markdown

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

@rodriguez46p-ui

Copy link
Copy Markdown
Author

Addressed the remaining ClawSweeper findings in 29fbfcf.

What changed:

  • Bearer-token status now reports availableTransport: "bearer" with rawStatus: "bearer-token" instead of masquerading as xurl. The status text explicitly says xurl status was not probed.
  • Profile hydration treats both xurl and bearer as live X transports, while local still skips hydration.
  • Backup setup now fails closed before initializing or cloning backup Git state inside an existing non-home parent Git worktree, with a migration-style diagnostic instead of silently creating a nested repo.
  • Added regression tests for the explicit bearer status and nested-worktree backup guard.

Verification from this branch:

  • npx --yes node@25.8.1 ./node_modules/vitest/vitest.mjs run src/lib/xurl.test.ts src/lib/backup.test.ts src/lib/profile-hydration.test.ts — passed, 3 files / 96 tests.
  • corepack pnpm exec tsgo --noEmit — passed.
  • corepack pnpm exec oxfmt --check src/lib/xurl.ts src/lib/xurl.test.ts src/lib/backup.ts src/lib/backup.test.ts src/lib/types.ts src/lib/profile-hydration.ts — passed.
  • corepack pnpm exec oxlint src/lib/xurl.ts src/lib/xurl.test.ts src/lib/backup.ts src/lib/backup.test.ts src/lib/types.ts src/lib/profile-hydration.ts src/lib/profile-hydration.test.ts — exit 0; still reports the pre-existing no-control-regex warning in src/lib/xurl.ts.
  • git diff --check — passed.

I still cannot provide live X API/LaunchAgent proof from this cron host without maintainer credentials/runtime setup, but the two concrete correctness blockers now have behavior coverage and implementation changes.

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented Jun 27, 2026

Copy link
Copy Markdown

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

@rodriguez46p-ui

Copy link
Copy Markdown
Author

Addressed the runtime status schema drift in a2d136c.

What changed:

  • queryEnvelopeSchema now accepts availableTransport: "bearer" in the transport status envelope.
  • Added API contract regression coverage that parses a bearer-token /api/status-style envelope with rawStatus: "bearer-token".

Verification from this branch:

  • corepack pnpm exec vitest run src/lib/api-contracts.test.ts src/lib/xurl.test.ts src/lib/backup.test.ts src/lib/profile-hydration.test.ts — passed, 4 files / 105 tests.
  • corepack pnpm exec tsgo --noEmit — passed.
  • corepack pnpm run lint — passed, 0 warnings/errors.
  • corepack pnpm exec oxfmt --check src/lib/archive-finder.ts src/lib/archive-import.test.ts src/lib/backup.test.ts src/lib/backup.ts src/lib/xurl.test.ts src/lib/xurl.ts src/test/setup.ts package.json pnpm-lock.yaml .gitattributes — passed.
  • git diff --check — passed, with Windows LF→CRLF working-copy warnings only.

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented Jun 28, 2026

Copy link
Copy Markdown

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

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

Labels

merge-risk: 🚨 auth-provider 🚨 Merging this PR could break OAuth, tokens, provider routing, model choice, or credentials. merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. P2 Normal priority bug or improvement with limited blast radius. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant