Skip to content

fix: bound portable subscriber refresh without background repacks - #163

Merged
steipete merged 5 commits into
mainfrom
fix/portable-refresh-safety-20260829
Aug 29, 2026
Merged

fix: bound portable subscriber refresh without background repacks#163
steipete merged 5 commits into
mainfrom
fix/portable-refresh-safety-20260829

Conversation

@steipete

@steipete steipete commented Aug 29, 2026

Copy link
Copy Markdown
Contributor

Summary

Fix portable initialization against manifest-backed gzip archives while keeping the configured database path logical (.db). Initialization validates portable arguments before Git work and validates the selected artifact before saving configuration.

Add gitcrawl portable refresh for scheduled subscribers. Canonical cross-process ownership, explicit maintenance suppression, frozen-artifact validation, timeout, free-space and growth limits protect the update boundary. The strict path does not reset, reclone, prune, remove sidecars, or regenerate configuration. Legacy recovery remains separate.

Preserve genuine writable runtime data across refresh and ordinary reads. Disposable raw replicas record their validated source digest and use immutable SQLite reads, so reader-created sidecars cannot incorrectly turn them into permanently stale local state. Status reports the actual usable gzip artifact or runtime rather than an absent logical file.

Reject credential-bearing remote URLs before Git execution while retaining legitimate SSH usernames and SCP syntax. This addresses the remote-userinfo review finding without breaking normal SSH transport identity.

Validation

  • Final CI for 46ec75e passed on Ubuntu and macOS, including formatting, tidy, vet, vulnerability/deadcode scans, tests, release-script checks and snapshot builds. Whole-project coverage is 85.1% on both platforms, with the existing 85% gate unchanged. CodeQL, Docker and secret scanning also passed.
  • Independent native macOS, source-blind acceptance exercised the actual CLI, Git and SQLite against fresh synthetic subscriber stores. All twelve broad contract clauses passed: raw/gzip transitions, real update/no-op, maintenance suppression with a positive control, invalid-generation preservation, dirty/index/ignored-file refusal, divergence, capacity, timeout/cancellation, ownership, local-write retention and truthful status.
  • The final credential guard passed native argument-admission and compatibility regressions, including refusal before configuration/Git work and retained SSH username support.
  • The final targeted source-blind replay passed all four input/error clauses: 22 forbidden URL forms refused before Git with no leakage or state changes; eight legitimate SSH/SCP/HTTPS/file input controls admitted; real local-store update/no-op verified; and missing-repository failures reported an actionable cause and guidance without raw diagnostics. The successful Git-stub controls prove input admission, not network authentication.

The synthetic stores contained no production archive data or credentials. Real fleet deployment follows the separately verified signed release; no production deployment is claimed by these tests.

The 0.9.3 changelog is dated. Publication remains owned by the official release workflow after landing.

@clawsweeper

clawsweeper Bot commented Aug 29, 2026

Copy link
Copy Markdown

🦞👀
ClawSweeper picked this up.

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

@clawsweeper clawsweeper Bot added merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. merge-risk: 🚨 security-boundary 🚨 Merging this PR could weaken sandboxing, authorization, credentials, or sensitive data. 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. labels Aug 29, 2026
@clawsweeper

clawsweeper Bot commented Aug 29, 2026

Copy link
Copy Markdown

Codex review: needs real behavior proof before merge. Reviewed August 29, 2026, 10:49 AM ET / 14:49 UTC.

ClawSweeper review

What this changes

The PR adds a strict portable-subscriber refresh command, hardens portable initialization and Git execution, preserves runtime mirrors, and adds related tests and documentation.

Merge readiness

Blocked until real behavior proof from a real setup is added - 7 items remain

Keep this draft PR open: current main does not contain its portable-refresh implementation, but the release heading is premature and real behavior proof remains required before merge.

Priority: P2
Reviewed head: 46ec75ea6b3e0a55fe83bc8fba4ad29af9d52b1c
Owner decision: Required. See Decision needed.

Review scores

Measure Result What it means
Overall readiness 🦪 silver shellfish (2/6) The implementation has strong fixture coverage, but an unresolved release-metadata defect and missing real authority-boundary proof keep it from merge readiness.
Proof confidence 🦪 silver shellfish (2/6) Needs real behavior proof before merge: Authority-chain proof required: the production refresh owner validates the configured origin before calling Git fetch, and the fixture suite exercises related refusals, but the PR body states independent proof is pending and supplies no redacted real-subscriber trace for either successful refresh or rejection before network I/O. Add terminal output, logs, or a recording with private URLs, IPs, and credentials redacted; updating the PR body should trigger review, or a maintainer can comment @clawsweeper re-review. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Patch quality 🦐 gold shrimp (3/6) Security review found an item that needs attention.

Verification

Check Result Evidence
Real behavior Needs proof Needs real behavior proof before merge: Authority-chain proof required: the production refresh owner validates the configured origin before calling Git fetch, and the fixture suite exercises related refusals, but the PR body states independent proof is pending and supplies no redacted real-subscriber trace for either successful refresh or rejection before network I/O. Add terminal output, logs, or a recording with private URLs, IPs, and credentials redacted; updating the PR body should trigger review, or a maintainer can comment @clawsweeper re-review. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Evidence reviewed 7 items Introduced portable refresh path: The PR introduces the scheduled refresh command and its configured-store admission path; current main is the pinned base, so this behavior is not already on main.
Origin checked before network fetch: The checkout origin is compared with the supplied expected origin during inspection, before the refresh path reaches Git fetch.
Credential URL hardening: Portable remote validation rejects HTTP(S) userinfo and passwords while retaining SSH usernames.
Findings 1 actionable finding [P2] Keep the 0.9.3 section unreleased
Security Needs attention Prove mismatched origins cannot reach fetch: The new refresh path consumes a configured origin and then invokes Git fetch; provide a redacted production-path trace that shows a mismatched origin is refused before that final network side effect.

How this fits together

Gitcrawl reads portable Git checkouts containing archive databases, while writable operations use a separate runtime mirror. This PR controls how a subscriber validates its configured origin, updates the checkout, and promotes a verified archive to that mirror.

flowchart LR
  A[Configured portable store] --> B[Subscriber refresh]
  C[Expected origin] --> D[Origin validation]
  B --> D
  D --> E[Fetch and artifact checks]
  E --> F[Clean checkout advancement]
  E --> G[Runtime mirror preservation]
  F --> H[Gitcrawl archive reads]
  G --> H
Loading

Decision needed

Question Recommendation
Should the intentional rejection of existing credential-bearing portable-store URLs proceed with an explicit credential-helper migration note? Confirm rejection and document migration: Keep URL credentials refused and add a concise upgrade note directing existing scripts to Git credential helpers.

Why: This security hardening can stop existing initialization scripts, so maintainers must confirm the upgrade contract rather than infer it from the implementation.

Before merge

  • Add real behavior proof - Needs real behavior proof before merge: Authority-chain proof required: the production refresh owner validates the configured origin before calling Git fetch, and the fixture suite exercises related refusals, but the PR body states independent proof is pending and supplies no redacted real-subscriber trace for either successful refresh or rejection before network I/O. Add terminal output, logs, or a recording with private URLs, IPs, and credentials redacted; updating the PR body should trigger review, or a maintainer can comment @clawsweeper re-review. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
  • Keep the 0.9.3 section unreleased (P2) - Restore Unreleased until formal release preparation. The latest release is v0.9.2 and this draft explicitly says no deployment should be inferred, so a release date here would publish incorrect release metadata.
  • Resolve security concern: Prove mismatched origins cannot reach fetch - The new refresh path consumes a configured origin and then invokes Git fetch; provide a redacted production-path trace that shows a mismatched origin is refused before that final network side effect.
  • Resolve merge risk (P1) - Existing scripts that place HTTP(S) credentials in the portable-store URL will now be refused and need a credential-helper migration.
  • Resolve merge risk (P1) - There is no redacted real subscriber trace proving both successful refresh and mismatched-origin rejection before Git fetch reaches the network.
  • Complete next step (P2) - A maintainer must confirm the credential-URL upgrade contract, and the contributor must supply real behavior proof; neither is a safe automated repair task.

Findings

  • [P2] Keep the 0.9.3 section unreleased — CHANGELOG.md:3
  • [medium] Prove mismatched origins cannot reach fetch — internal/cli/portable_refresh.go:238
Agent review details

Security

Needs attention: The remote-validation design is source-backed, but its final network-effect boundary has no real after-fix evidence yet.

Review metrics

Metric Value Why it matters
Patch scope 31 files, +4202/-183 The change crosses CLI, portable runtime, platform process handling, tests, and operational documentation.
Production versus tests production +2027/-157, tests +1995/-24 The substantial implementation growth is paired with comparably substantial fixture coverage, but it still needs real subscriber proof.

Merge-risk options

Maintainer options:

  1. Correct release metadata and add real boundary proof (recommended)
    Keep the heading unreleased, state the credential-helper migration, and add a redacted real-subscriber trace covering a safe refresh plus origin mismatch refusal before network I/O.
  2. Pause the change
    Defer the PR if maintainers do not want portable initialization scripts with URL credentials to fail on upgrade.

Technical review

Best possible solution:

Keep the section unreleased until formal release preparation, document the credential-helper migration, and land only after a redacted real subscriber run proves the allowed and rejected origin paths.

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

Not applicable as a feature-and-fix PR: source fixtures cover raw/gzip transitions, preservation, and refusals, but they are not a current production bug reproduction.

Is this the best way to solve the issue?

No—not yet. The safety design is coherent, but release metadata must remain accurate and the remote-authority behavior needs real after-fix evidence.

Full review comments:

  • [P2] Keep the 0.9.3 section unreleased — CHANGELOG.md:3
    Restore Unreleased until formal release preparation. The latest release is v0.9.2 and this draft explicitly says no deployment should be inferred, so a release date here would publish incorrect release metadata.
    Confidence: 0.99

Overall correctness: patch is incorrect
Overall confidence: 0.95

AGENTS.md: not found in the target repository.

Codex review notes: model internal, reasoning high; reviewed against 56bf348dcad0.

Labels

Label justifications:

  • P2: The PR changes a bounded but operationally important portable subscriber workflow.
  • merge-risk: 🚨 compatibility: Portable initialization now rejects credential-bearing HTTP(S) URLs that existing scripts may use.
  • merge-risk: 🚨 security-boundary: Configured remote identity is validated before a network fetch, and final-effect evidence is still required for that boundary.
  • rating: 🦪 silver shellfish: Overall readiness is 🦪 silver shellfish; proof is 🦪 silver shellfish and patch quality is 🦐 gold shrimp.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: Authority-chain proof required: the production refresh owner validates the configured origin before calling Git fetch, and the fixture suite exercises related refusals, but the PR body states independent proof is pending and supplies no redacted real-subscriber trace for either successful refresh or rejection before network I/O. Add terminal output, logs, or a recording with private URLs, IPs, and credentials redacted; updating the PR body should trigger review, or a maintainer can comment @clawsweeper re-review. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Evidence

Security concerns:

  • [medium] Prove mismatched origins cannot reach fetch — internal/cli/portable_refresh.go:238
    The new refresh path consumes a configured origin and then invokes Git fetch; provide a redacted production-path trace that shows a mismatched origin is refused before that final network side effect.
    Confidence: 0.88

What I checked:

  • Introduced portable refresh path: The PR introduces the scheduled refresh command and its configured-store admission path; current main is the pinned base, so this behavior is not already on main. (internal/cli/portable_refresh.go:44, 46ec75ea6b3e)
  • Origin checked before network fetch: The checkout origin is compared with the supplied expected origin during inspection, before the refresh path reaches Git fetch. (internal/cli/portable_checkout.go:63, 46ec75ea6b3e)
  • Credential URL hardening: Portable remote validation rejects HTTP(S) userinfo and passwords while retaining SSH usernames. (internal/cli/portable_owner.go:198, 0ad8e3b22674)
  • Premature release heading: The branch changes the current 0.9.3 section from Unreleased to a date although the PR body explicitly says no release or deployment should be inferred. (CHANGELOG.md:3, 46ec75ea6b3e)
  • Feature history: Portable refresh and the current ownership/scope work were introduced by Peter Steinberger; the prior gzip artifact generation was introduced by Ayaan Zaidi. (internal/cli/runtime.go:318, 6ecb698f5a41)
  • Release status: No release tag contains the PR head; the latest supplied release is v0.9.2, which predates this branch. (CHANGELOG.md:13, 46ec75ea6b3e)

Likely related people:

  • Peter Steinberger: Introduced the refresh implementation and has the strongest sustained history across portable runtime, repair, and integrity work. (role: recent portable-runtime contributor; confidence: high; commits: dbeee5bb3741, 6ecb698f5a41, ff6b2b41861d; files: internal/cli/runtime.go, internal/cli/app.go, internal/cli/portable_refresh.go)
  • Ayaan Zaidi: Introduced the prior gzip portable artifact generation that this PR extends and fixes around. (role: gzip artifact-generation contributor; confidence: medium; commits: 73ea39cc0a07; files: internal/cli/runtime.go, docs/portable-stores.md)
  • Vincent Koc: Introduced earlier gzip-backed portable-store support relevant to the raw/gzip transition coverage. (role: adjacent portable-format contributor; confidence: medium; commits: 2b4746c97ed7; files: internal/cli/runtime.go)

Rank-up moves

Optional improvements that raise the rating; they are not merge blockers.

  • Restore the 0.9.3 heading to Unreleased until formal release preparation.
  • Add a concise migration note for scripts that currently use URL credentials.
  • Provide redacted final-effect proof for an allowed subscriber refresh and for a mismatched origin refused before Git fetch.

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.

History

Review history (4 earlier review cycles)
  • reviewed 2026-08-29T05:34:49.920Z sha dbeee5b :: needs real behavior proof before merge. :: [P2] Reject credentials in portable remote URLs
  • reviewed 2026-08-29T12:37:46.684Z sha 6ecb698 :: needs real behavior proof before merge. :: [P2] Reject credentials in portable remote URLs
  • reviewed 2026-08-29T13:48:01.429Z sha a8f538a :: needs real behavior proof before merge. :: [P2] Reject HTTP(S) URL userinfo
  • reviewed 2026-08-29T14:12:01.984Z sha 0ad8e3b :: needs real behavior proof before merge. :: [P1] Provide an upgrade path for credential-bearing init URLs | [P2] Validate the configured origin before fetching

@clawsweeper clawsweeper Bot added rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. and removed rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. labels Aug 29, 2026
@steipete
steipete marked this pull request as ready for review August 29, 2026 14:58
@steipete
steipete merged commit 47232a7 into main Aug 29, 2026
14 checks passed
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. merge-risk: 🚨 security-boundary 🚨 Merging this PR could weaken sandboxing, authorization, credentials, or sensitive data. P2 Normal priority bug or improvement with limited blast radius. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. 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