Skip to content

fix(exports): keep private output outside source checkouts - #19

Merged
vincentkoc merged 1 commit into
mainfrom
fix/private-export-output-boundary
Sep 12, 2026
Merged

vincentkoc merged 1 commit into
mainfrom
fix/private-export-output-boundary

Conversation

@vincentkoc

@vincentkoc vincentkoc commented Sep 11, 2026 •

Copy link
Copy Markdown
Member

Offline npm:quality and telemetry:history exports could write private results into this source repository, making accidental commits possible. Reject destinations inside the executing checkout, its linked worktrees, or the input archive before creating output. Shared filesystem identity checks also close case-alias bypasses and keep nested repositories from hiding a containing checkout.

Unrelated private repositories remain supported, with existing private file modes and idempotency preserved. Update the examples and changelog, and ignore root captures/ and results/ directories as an additional precaution. This guard covers these two offline exporters; it does not prevent every possible way of publishing data.

Validation:

  • Hosted CI passed npm run check (vocabulary, typecheck, all 347 tests) and npx wrangler deploy --dry-run at 23f03ee1b088a200ff0c1fc819a57c16108b442c. The deployment job was skipped.
  • macOS actual-writer proof: 36 checkout/archive destinations rejected without output; 8 external/private destinations accepted with private modes and history idempotency preserved.
  • Syntax checks, whitespace checks, and independent review passed.

@clawsweeper

clawsweeper Bot commented Sep 11, 2026 •

Copy link
Copy Markdown

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

ClawSweeper review complete

ClawSweeper finished reviewing this revision. The review result is being finalized.

View the workflow run.

@clawsweeper clawsweeper Bot added 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. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. labels Sep 11, 2026
@clawsweeper

clawsweeper Bot commented Sep 11, 2026

Copy link
Copy Markdown

Codex review: blocked before merge. Reviewed September 11, 2026, 6:57 PM ET / 22:57 UTC.

ClawSweeper review

What this changes

Both offline exporters reject output inside their source checkout, linked worktrees, or input archive using filesystem identity checks, with corresponding tests and documentation.

Merge readiness

⛔ Blocked before merge - 3 items remain

This remains useful: current main lacks the checkout boundary, and no merged replacement supplies it. No blocking code defect was found; the intentional restriction on existing output destinations needs maintainer acceptance.

Priority: P2
Reviewed head: 23f03ee1b088a200ff0c1fc819a57c16108b442c
Owner decision: Required. See Decision needed.

Review scores

Measure Result What it means
Overall readiness 🐚 platinum hermit (4/6) A focused shared guard with substantial boundary coverage and no identified blocking defect; operator compatibility remains an explicit landing choice.
Proof confidence 🌊 off-meta tidepool Not applicable: The MEMBER-authored PR is exempt from the contributor proof gate; its body reports macOS runs of both actual writers rejecting protected destinations and preserving external writes, modes, and history reuse, without a separately inspectable transcript.
Patch quality 🐚 platinum hermit (4/6) No actionable review findings were identified.

Verification

Check Result Evidence
Real behavior Not applicable Not applicable: The MEMBER-authored PR is exempt from the contributor proof gate; its body reports macOS runs of both actual writers rejecting protected destinations and preserving external writes, modes, and history reuse, without a separately inspectable transcript.
Evidence reviewed 6 items Current main still lacks the guard: The current main writer checks lexical archive containment but does not reject source checkouts. The branch API confirmed main remains 6acfa31; release and local tag queries returned no entries establishing a shipped replacement.
Shared filesystem boundary: The helper compares device/inode identities and follows Git common-directory metadata across destination ancestors, including nested foreign repositories. Both production writers invoke it before mkdirSync; existing exclusive writes, private modes, and history reuse checks remain intact.
Intentional operator compatibility change: The previous documented npm comparison command wrote to ./results/run-01. The new guard rejects that destination when run in this repository, and documentation now directs operators to external private paths.
Findings None None.
Security None None.

How this fits together

Telemetry's offline analysis commands read saved captures and produce private reports without contacting the deployed Worker or external services. The changed destination guard runs immediately before report directories and files are created.

flowchart TD
 A[Saved captures] --> B[Offline analysis]
 B --> C[Output destination guard]
 D[Checkout and archive identities] --> C
 C -->|Protected destination| E[Reject before writing]
 C -->|External destination| F[Private report files]
Loading

Decision needed

Question Recommendation
Accept mandatory rejection of checkout-local exports, including commands copied from the previous ./results example? Accept the private-output boundary: Land the restriction with the documented external destinations and update any existing operator commands before adopting it.

Why: The restriction is deliberate and documented, but accepting the interruption of existing operator commands is a compatibility decision rather than a code correctness judgment.

Before merge

  • Resolve merge risk (P1) - Existing export commands targeting this checkout or its linked worktrees will stop with an error after upgrade, including the formerly documented ./results destination; operators must select an external private output path.
  • Complete next step (P2) - Confirm acceptance of the checkout-local output restriction and readiness to move affected operator commands to external private paths.
  • Resolve maintainer decision - Resolve the maintainer decision shown above before merge.
Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Production and test growth Production +81/-11 (net +70); tests/helpers +233/-3 (net +230) Production growth centralizes the two writers' filesystem boundary checks, with most added lines devoted to regression coverage.

Merge-risk options

Maintainer options:

  1. Accept the documented destination restriction (recommended)
    Confirm that existing checkout-local export commands can be moved to external private paths before adopting the guard.
  2. Wait for the operator transition
    Pause landing while any dependent scripts still require checkout-local output.

Technical review

Best possible solution:

Keep the shared privacy guard with explicit acceptance of the destination restriction and an operator transition to external private paths.

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

Yes, from source: current main permits a new checkout-local destination outside the input archive. This review did not execute the writers.

Is this the best way to solve the issue?

Yes, subject to accepting the compatibility change: one shared filesystem-identity guard covers both writers without changing report formats or adding dependencies.

AGENTS.md: not found in the target repository.

Codex review notes: model internal, reasoning medium; reviewed against 6acfa31f5dad.

Labels

Label changes:

  • add P2: This is bounded privacy hardening for two offline operator commands.
  • add merge-risk: 🚨 compatibility: Previously accepted checkout-local output paths now fail and require operator command changes.
  • add rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🌊 off-meta tidepool and patch quality is 🐚 platinum hermit.
  • add status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Not applicable: The MEMBER-authored PR is exempt from the contributor proof gate; its body reports macOS runs of both actual writers rejecting protected destinations and preserving external writes, modes, and history reuse, without a separately inspectable transcript.

Label justifications:

  • P2: This is bounded privacy hardening for two offline operator commands.
  • merge-risk: 🚨 compatibility: Previously accepted checkout-local output paths now fail and require operator command changes.
  • rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🌊 off-meta tidepool and patch quality is 🐚 platinum hermit.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Not applicable: The MEMBER-authored PR is exempt from the contributor proof gate; its body reports macOS runs of both actual writers rejecting protected destinations and preserving external writes, modes, and history reuse, without a separately inspectable transcript.

Evidence

What I checked:

  • Current main still lacks the guard: The current main writer checks lexical archive containment but does not reject source checkouts. The branch API confirmed main remains 6acfa31; release and local tag queries returned no entries establishing a shipped replacement. (scripts/lib/npm-quality.mjs:1031, 6acfa31f5dad)
  • Shared filesystem boundary: The helper compares device/inode identities and follows Git common-directory metadata across destination ancestors, including nested foreign repositories. Both production writers invoke it before mkdirSync; existing exclusive writes, private modes, and history reuse checks remain intact. (scripts/lib/private-export-path.mjs:48, 23f03ee1b088)
  • Intentional operator compatibility change: The previous documented npm comparison command wrote to ./results/run-01. The new guard rejects that destination when run in this repository, and documentation now directs operators to external private paths. (docs/npm-comparison.md:6, 23f03ee1b088)
  • Regression coverage and reported runtime validation: Tests exercise both writers from source and linked worktrees, protected destinations, unrelated repositories, filesystem case semantics, private modes, and history idempotency. The captured PR body additionally reports macOS actual-writer validation with 36 rejected and 8 accepted destinations, plus hosted checks at the reviewed head. No separate runtime transcript or media was supplied; tests were not executed during this read-only review. (test/telemetry-history.test.mjs:517, 23f03ee1b088)
  • Feature-history routing: The writer line traces to 5eff9fe. Its raw parent record and parent tree were inspected. GitHub confirms vincentkoc authored the merged exporter contributions at feat(telemetry): export archived hourly report history #17 and feat(analysis): add offline npm comparison tooling #14. Broader --follow queries encountered unavailable historical objects; targeted history and GitHub metadata supplied the relevant routing evidence. (scripts/lib/telemetry-history.mjs:405, 5eff9fe714d9)
  • Related capture work is distinct: feat(telemetry): add aggregate capture pilot #18 adds an aggregate capture command and explicitly leaves these exporters unchanged; it is not a replacement for this destination guard.

Likely related people:

  • Vincent Koc: Raw commit 5eff9fe adds scripts/lib/telemetry-history.mjs:404 relative to its recorded parents. This identifies author metadata, not feature responsibility or a PR merger. (role: source-line author; confidence: high; commits: 5eff9fe714d9; files: scripts/lib/telemetry-history.mjs)

Rating scale

Score Internal tier Crab rank Meaning
6/6 S 🦀 challenger crab Exceptional readiness
5/6 A 🦞 diamond lobster Very strong readiness
4/6 B 🐚 platinum hermit Good normal PR; ordinary maintainer review
3/6 C 🦐 gold shrimp Useful, but confidence is limited
2/6 D 🦪 silver shellfish Proof or implementation needs work
1/6 F 🧂 unranked krab Not merge-ready
N/A NA 🌊 off-meta tidepool Rating does not apply

Overall follows the weaker of proof and patch quality.
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.

Workflow

  • 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.

@vincentkoc

Copy link
Copy Markdown
Member Author

Maintainer decision: accept the intentional checkout-local output restriction in 23f03ee1b088a200ff0c1fc819a57c16108b442c.

Commands using the former ./results example must select an external private output directory before running either exporter. The updated examples and changelog document that transition; unrelated private repositories remain supported. This is the intended privacy boundary, including for linked worktrees.

This accepts the compatibility risk and resolves the three maintainer-decision/transition items in the ClawSweeper review. Source is unchanged, and exact-head CI passed all 347 tests, vocabulary validation, typechecking, and the deployment dry run.

@vincentkoc vincentkoc self-assigned this Sep 12, 2026
@vincentkoc
vincentkoc marked this pull request as ready for review September 12, 2026 03:24
@vincentkoc
vincentkoc merged commit bd314c2 into main Sep 12, 2026
6 checks passed
@vincentkoc
vincentkoc deleted the fix/private-export-output-boundary branch September 25, 2026 11:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant