Skip to content

feat(cli): PM-unified github store layout with shallow clones#67

Merged
amondnet merged 18 commits into
mainfrom
docs/github-store-pm-unified-idea
Apr 12, 2026
Merged

feat(cli): PM-unified github store layout with shallow clones#67
amondnet merged 18 commits into
mainfrom
docs/github-store-pm-unified-idea

Conversation

@amondnet

@amondnet amondnet commented Apr 11, 2026

Copy link
Copy Markdown
Contributor

Summary

Refactors the <ASK_HOME>/github/ store to use a PM-unified nested layout
(github.com/<owner>/<repo>/<tag>/) with independent shallow clones, replacing
the legacy bare-clone subsystem that had five structural correctness defects.

Key changes

  • Store layout: github/<host>/<owner>/<repo>/<tag>/ — each entry is an independent shallow clone with .git/ stripped and commit SHA captured
  • Schema: strict mutable-ref validation (rejects main/master/HEAD/etc.), ResolvedEntry.commit and FetchResult.storeSubpath fields
  • Source: cloneAtTag with v-prefix fallback (never emits vv1.2.3), docsPath traversal guard hardened with realpathSync
  • Store integrity: verifyEntry guard on store-hit short-circuit, quarantineEntry for corrupt entries, STORE_VERSION file
  • Legacy migration: detection of old github/db/ and github/checkouts/ dirs, ask cache clean --legacy command
  • CLI flags: --allow-mutable-ref to bypass strict ref validation
  • Link/ref mode: storeSubpath support so symlinks point at the docs subdirectory, not the repo root

Track

  • Idea: .please/docs/ideas/github-store-pm-unified.md
  • Track: .please/docs/tracks/active/github-store-pm-unified-20260411/

Test plan

  • Unit tests for githubStorePath segment validation
  • Unit tests for strict/lax AskJsonSchema ref validation
  • Integration tests for saveDocs symlink/copy fallback
  • Integration tests for concurrent install and .git/ strip
  • Store-hit guard, vv-ref prevention, and quarantineEntry invariant tests
  • docsPath traversal guard tests (symlink bypass prevention)

Captures the recommended direction (D6, "PM-Unified Layout") for a
future refactor of the <ASK_HOME>/github/ store layout, synthesized
from a brainstorm session and vendor/opensrc reference analysis.

Idea-stage only; implementation will be tracked in a separate
/please:new-track run.
@dosubot dosubot Bot added the size:L This PR changes 100-499 lines, ignoring generated files. label Apr 11, 2026
@cloudflare-workers-and-pages

cloudflare-workers-and-pages Bot commented Apr 11, 2026

Copy link
Copy Markdown

Deploying ask-registry with  Cloudflare Pages  Cloudflare Pages

Latest commit: bf8f1df
Status:🚫  Build failed.

View logs

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

No issues found across 1 file

Auto-approved: This is a documentation-only change adding an architectural proposal/one-pager to the docs folder. It has no impact on source code or production logic.

Architecture diagram
sequenceDiagram
    participant CLI as CLI (ask install)
    participant VAL as Validation Layer
    participant GS as GitHub Source
    participant Git as Git Binary
    participant Store as Store (~/.ask/github/...)

    Note over CLI,Store: NEW: Proposed Unified Store Flow

    CLI->>VAL: Validate package ref
    alt NEW: Ref is mutable (main, master, etc.)
        VAL-->>CLI: Reject (Error: Suggest tag/SHA or --allow-mutable-ref)
    else Ref is tag-like or SHA
        VAL-->>CLI: Proceed
    end

    CLI->>GS: fetch(owner, repo, ref)
    
    Note over GS,Store: CHANGED: Check nested path <host>/<owner>/<repo>/<tag>
    GS->>Store: Check if path exists
    
    alt Store Hit
        GS->>Store: NEW: verifyEntry(path)
        alt Integrity Check Fails
            GS->>Store: NEW: Quarantine corrupt entry
        else Integrity Check Passes
            GS-->>CLI: Return FetchResult (with storeSubpath)
        end
    end

    opt Store Miss or Verification Failed
        GS->>Git: CHANGED: Shallow clone --depth 1 --branch <tag>
        Git-->>GS: Clone to tmpDir
        GS->>GS: NEW: Remove .git/ metadata
        GS->>Store: Atomic move tmpDir to nested store path
    end

    GS-->>CLI: Return FetchResult (NEW: includes storeSubpath)
    
    Note over CLI,Store: Materialization Phase
    alt Link Mode
        CLI->>Store: CHANGED: Symlink target + storeSubpath
    else Ref Mode
        CLI->>Store: CHANGED: Resolve Agents path + storeSubpath
    end
Loading

@codecov

codecov Bot commented Apr 11, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 84.77011% with 53 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
packages/cli/src/store/cache.ts 76.53% 23 Missing ⚠️
packages/cli/src/sources/github.ts 90.66% 14 Missing ⚠️
packages/cli/src/install.ts 72.00% 7 Missing ⚠️
packages/cli/src/store/index.ts 87.27% 7 Missing ⚠️
packages/cli/src/storage.ts 75.00% 2 Missing ⚠️

📢 Thoughts on this report? Let us know!

@dosubot dosubot Bot added status/draft Draft status:proposal Proposal for discussion type:docs Documentation improvements labels Apr 11, 2026
Track spec and plan for restructuring <ASK_HOME>/github/ to an
opensrc-style flat nested layout. Eliminates five correctness defects
(owner__repo collisions, FETCH_HEAD races, ref containing /, db/ GC,
concurrent FETCH_HEAD overwrites) structurally plus two spot defects
(link/ref subpath mismatch, missing verifyEntry guard).

Refs #68
@dosubot dosubot Bot added size:XL This PR changes 500-999 lines, ignoring generated files. and removed size:L This PR changes 100-499 lines, ignoring generated files. labels Apr 11, 2026
Add optional commit: GitCommitShaField (40-hex) on ResolvedEntry so
github installs can record the SHA that a tag resolved to at install
time. FetchResult.storeSubpath (TS-only interface field) carries the
docsPath subdirectory through the install pipeline so link/ref
materialization can point at the docs tree instead of the repo root.

Test matrix for commit field: accepts 40-hex SHA, accepts absence,
rejects non-hex, rejects short strings.

Part of T001 for track github-store-pm-unified-20260411.
Refs #68

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

0 issues found across 4 files (changes from recent commits).

Auto-approved: This PR is strictly limited to documentation and project tracking (idea doc, spec, and plan) within the .please/docs directory. No source code or production logic is modified.

Add a tag-like ref heuristic to standalone github entries: accepts
40-char hex SHA, v?semver, and tag-like strings containing a dot or
digit; rejects main/master/develop/trunk/HEAD/latest and bare
single-word refs. Error messages point at --allow-mutable-ref for
the CLI escape hatch (wired up in T008).

Export createAskJsonSchema({ strictRefs }) factory, with AskJsonSchema
staying as the strict default and LaxAskJsonSchema available for
callers that need to accept mutable refs (CI, test fixtures).

Updates the existing 'ask add github' test to use v1.0.0 instead of
main — the test verifies append behavior, the ref value is incidental.

Part of T002 for track github-store-pm-unified-20260411.
Refs #68
Add githubStorePath(askHome, host, owner, repo, tag) which resolves
to <askHome>/github/<host>/<owner>/<repo>/<tag>/ — the PM-style nested
layout that replaces the flattened <owner>__<repo> convention used by
githubDbPath and githubCheckoutPath.

Each input segment is rejected if it contains '..' / '/' / '\' or is
empty. assertContained at the github/ subdirectory guards the final
path against deeper escapes. The legacy helpers (githubDbPath,
githubCheckoutPath) are kept for this commit — they will be removed
together with the bare-clone subsystem in T004 and github source
rewrite in T005.

Part of T003 for track github-store-pm-unified-20260411.
Refs #68
Replace the bare-clone subsystem + flattened owner__repo layout with
opensrc-style per-tag shallow clones into a nested path:

  <askHome>/github/github.com/<owner>/<repo>/<tag>/

Each github install is now an independent shallow clone via
'git clone --depth 1 --branch <tag> --single-branch' into a temp dir,
followed by .git/ removal, cpDirAtomic into githubStorePath, and
stampEntry for verifyEntry guards. The commit SHA is captured via
'git rev-parse HEAD' before .git/ removal and surfaced in
FetchResult.meta.commit (40-char hex).

Tag fallback: when the user requests a bare version like '1.0.0',
the clone tries '1.0.0' first then 'v1.0.0'. Refs already starting
with 'v' are tried only once — never 'vv1.2.3'. The winning candidate
becomes the store key so caches remain deduplicated.

Store-hit path runs verifyEntry() on the nested directory for every
candidate; corrupted entries are moved to <askHome>/.quarantine/
and a fresh clone replaces them.

The fetchFromTarGz fallback (git-less environments) is preserved but
rewritten to write to the new layout and return storeSubpath. The
bare-clone subsystem (store/github-bare.ts + test) is deleted.

FetchResult.storeSubpath now carries the docsPath subdirectory so
downstream link/ref materialization (T007) can point at the docs
tree instead of the repo root.

Schema: createAskJsonSchema factory returns a concrete union type
instead of ZodTypeAny so CLI type inference survives the factory
indirection.

BREAKING: existing <askHome>/github/{db,checkouts}/ directories are
not migrated. T009 adds 'ask cache clean --legacy' plus a detection
warning on install start.

Part of T004 and T005 for track github-store-pm-unified-20260411.
Refs #68
Wrap the npm store-hit short-circuit in install.ts around line 274
with a verifyEntry() check. A store entry whose stamp is missing or
whose content hash does not match is moved to <askHome>/.quarantine/
and the install falls through to a fresh fetch from the source
adapter. The same quarantineEntry helper is used by the github source
store-hit path (T005) — it moves to the shared store/index.ts.

Propagate result.meta.commit into ResolvedEntry.commit so github
installs record the 40-char hex SHA the tag resolved to at install
time. For npm/web/llms-txt meta.commit is unset, so the field stays
undefined.

Github source switches from its private quarantineEntry to the shared
one now exposed from store/index.ts.

End-to-end quarantine + commit propagation tests land in T010 as
integration scenarios against the full install pipeline.

Part of T006 for track github-store-pm-unified-20260411.
Refs #68
In saveDocs, compute effectivePath = path.join(storePath, storeSubpath ?? '')
and use it as the symlink target (link mode) or returned path (ref
mode). For npm/web/llms-txt, storeSubpath is undefined and the
behaviour matches the pre-T007 contract.

generateAgentsMd joins storePath + storeSubpath when rendering ref-mode
entries, so AGENTS.md points at the actual docs directory instead of
the repo root for github entries with a docsPath.

ResolvedEntry gains an optional storeSubpath field so the subpath
persists across installs (required for ref mode on subsequent reads).

Unit tests cover all four paths:
- ref mode with/without storeSubpath
- link mode with/without storeSubpath (with copy fallback)

Part of T007 for track github-store-pm-unified-20260411.
Refs #68
Add the --allow-mutable-ref flag to ask install and ask add. When
set, the install orchestrator skips the strict ref validation pass
on ask.json so entries with refs like main/master/HEAD/latest can
be installed (CI pipelines, test fixtures, quick spikes).

Architecture: read/write go through the lax schema unconditionally
so internal readers (listDocs, generateAgentsMd, manageIgnoreFiles,
etc.) never need to know about the escape hatch. Strict validation
happens once, at the CLI entry point in runInstall, via a new
validateAskJsonStrict helper. This inversion prevents the earlier
whack-a-mole where every internal read that touched ask.json needed
its own strict-or-lax branching.

Add a CLI integration test for the happy path (ask add ...
--ref main --allow-mutable-ref writes the entry). The reject path
is already covered by the schema-level tests added in T002.

Part of T008 for track github-store-pm-unified-20260411.
Refs #68
@dosubot dosubot Bot added size:XXL This PR changes 1000+ lines, ignoring generated files. and removed size:XL This PR changes 500-999 lines, ignoring generated files. labels Apr 11, 2026
Add the final pieces of the migration story from the pre-v2 github
store layout:

- writeStoreVersion / readStoreVersion in store/index.ts: write
  <askHome>/STORE_VERSION='2' on install start.
- detectLegacyLayout / cacheCleanLegacy in store/cache.ts: identify
  pre-v2 paths (github/db, github/checkouts) and optionally wipe
  them. Idempotent on a clean tree.
- runInstall: emit a one-line warning on install start when any
  legacy path is present, pointing at 'ask cache clean --legacy'.
  Always writes STORE_VERSION after the warning.
- cacheLs rewrite: walks the new nested layout (github/<host>/<owner>/
  <repo>/<tag>/) AND the legacy layout (github/checkouts/<owner>__<repo>
  /<ref>/) side by side. Legacy entries are tagged with
  legacy: true and rendered with a '(legacy) ' key prefix so users
  can spot them without a separate --legacy flag (Q3).
- ask cache clean --legacy CLI subcommand: wraps cacheCleanLegacy
  with an idempotent message and an info-level fast path when
  nothing to clean.

Tests cover:
- cacheLs for new layout, legacy layout, and mixed.
- detectLegacyLayout + cacheCleanLegacy unit tests including
  idempotency.
- STORE_VERSION read/write round-trip.

Part of T009 for track github-store-pm-unified-20260411.
Refs #68

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

2 issues found across 17 files (changes from recent commits).

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="packages/cli/test/cli/commands.test.ts">

<violation number="1" location="packages/cli/test/cli/commands.test.ts:103">
P3: This test swallows the only signal that mutable-ref validation succeeded, so it can pass even if `--allow-mutable-ref` regresses. Assert a post-install effect instead of ignoring the command result.</violation>
</file>

<file name="packages/cli/test/storage.test.ts">

<violation number="1" location="packages/cli/test/storage.test.ts:94">
P2: The EPERM fallback assertion expects `README.md`, but copy mode only creates the passed-in files plus `INDEX.md`. This will fail on systems where symlink creation falls back.</violation>
</file>

Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.

Comment thread packages/cli/test/storage.test.ts Outdated
Comment thread packages/cli/test/cli/commands.test.ts Outdated
…T010)

Add the remaining integration-level coverage:

- Concurrent install of two different tags of the same repo
  (SC-2). Fires both fetches in parallel, asserts both land in
  distinct nested store paths, neither corrupts the other, both
  stamps verify, and content is correctly versioned. This is the
  structural defect-killer: no shared bare repo, no FETCH_HEAD
  race, no owner__repo collision.

- A3 shallow-clone .git/ audit: asserts that a materialized store
  entry contains no .git/ directory anywhere in the tree. The
  strip happens in cloneAtTag() before cpDirAtomic. A static grep
  for ".git" substrings in the source is too coarse (many
  unrelated files legitimately contain it: ignore files, repo URL
  parsers, comments), so the audit is expressed as a runtime
  smoke test against the actual store state.

Part of T010 for track github-store-pm-unified-20260411.
Refs #68
A2 (multi-tag usage): zero duplicate owner/repo entries with
different ref values in committed samples across 50 registry
entries and the single committed ask.json sample. Assumption holds;
no dedup layer needed.

A4 (heuristic false-positive rate): 14/16 test fixture refs accepted
by the strict heuristic. The two rejections (main, canary) are
intentional negative test cases. Real-world false-positive rate on
committed non-test fixtures is 0/1. Assumption holds.

Report lives at .please/docs/tracks/active/github-store-pm-unified-20260411/audits.md
and will be linked from the PR body.

Part of T011 for track github-store-pm-unified-20260411.
Refs #68
Update README, CLAUDE.md gotchas, and ARCHITECTURE.md to reflect the
new github store layout and the surrounding machinery:

README:
- Replace legacy github/db + github/checkouts tree with the new
  nested github/github.com/<owner>/<repo>/<tag>/ layout.
- Document STORE_VERSION and the ask cache clean --legacy migration
  command with a one-line warning story for upgraders.
- Update the 'ask src / ask docs' share-the-store paragraph to
  point at the new path.

ARCHITECTURE.md:
- Add a new 'Global Store' section covering the four source kinds,
  shallow-clone semantics, verifyEntry + quarantine, store-mode
  materialization, and the new ref validation / escape hatch story.

CLAUDE.md gotchas (five new entries):
- Nested layout + no bare-clone resurrection
- Strict ref validation + lax schema for internal reads
- FetchResult.storeSubpath flows to link/ref materialization
- verifyEntry guard on both npm and github store-hit paths
- STORE_VERSION + legacy detection + 'ask cache clean --legacy'

Part of T012 for track github-store-pm-unified-20260411.
Refs #68
All 12 tasks (T001-T012) implemented. Tests: 384 pass, 0 fail.
Lint: clean. Ready for review.

Refs #68

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

1 issue found across 13 files (changes from recent commits).

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="README.md">

<violation number="1" location="README.md:307">
P3: The new store-model sentence is inaccurate: not all source kinds use `<identity>@<version>` paths. This can mislead users troubleshooting cache paths.</violation>
</file>

Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.

Comment thread README.md Outdated
- Add containment guard in github source's extractDocsFromDir so a
  malicious docsPath cannot escape the clone temp dir and exfiltrate
  arbitrary files into .ask/docs/ or .claude/skills/
- Reject absolute paths and `..` traversal segments in the ask.json
  schema's docsPath field (both strict and lax variants), giving
  defense in depth at the schema layer
- Distinguish ENOENT from real filesystem errors across cache.ts
  helpers (safeIsDirectory, safeReaddir, dirSize, findResolvedJsonFiles)
  so EACCES/EIO surface via consola.warn instead of becoming invisible
  storage leaks
- Fix latent data-loss path in cacheGc: a transient statSync failure
  on an --older-than gated run no longer silently removes the entry;
  it now warns and keeps to avoid destroying data on a flaky FS
- Surface malformed resolved.json in findResolvedJsonFiles so users
  understand why referenced store entries disappear after GC
Add regression tests for three load-bearing invariants flagged by the
review loop as coverage gaps:

- quarantineEntry: direct unit tests verify the corrupted entry is
  moved under <askHome>/.quarantine/<ts>-<uuid>/ with content preserved
  (forensic inspection), that .quarantine/ is created lazily, and that
  successive calls produce distinct UUID-tagged directories
- github source cloneAtTag: negative assertion that a v-prefixed ref
  like v9.9.9 never produces a vv9.9.9 candidate. The existing fallback
  tests only cover the positive paths; this locks the startsWith('v')
  guard against future regressions
- install.ts npm store-hit: three integration tests covering (1) valid
  stamp short-circuits without network, (2) tampered stamp quarantines
  and falls through to fresh fetch, (3) missing .ask-hash is treated
  the same as tampering. CLAUDE.md calls this path load-bearing —
  "Never reintroduce a bare fs.existsSync(storeDir) check without the
  guard" — and these tests fail loudly if the verifyEntry guard is
  dropped

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

1 issue found across 6 files (changes from recent commits).

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="packages/cli/src/sources/github.ts">

<violation number="1" location="packages/cli/src/sources/github.ts:327">
P1: The new docsPath guard is bypassable via symlinks because it uses `path.resolve` instead of realpath resolution.</violation>
</file>

Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.

Comment thread packages/cli/src/sources/github.ts Outdated
@amondnet amondnet changed the title docs(ideas): add github-store-pm-unified one-pager feat(cli): PM-unified github store layout with shallow clones Apr 12, 2026
- Fix EPERM fallback assertion in storage.test.ts to check INDEX.md (always written by copy mode) instead of README.md (not written when files=[])
- Harden --allow-mutable-ref test to rethrow validation errors so flag regressions are not silently swallowed by the network-error catch
- Use fs.realpathSync in github.ts docsPath guard to prevent symlink bypass attacks
- Correct README.md store-model description: github entries use nested host/owner/repo/tag/ paths, not the @Version format

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

0 issues found across 4 files (changes from recent commits).

Requires human review: Large structural refactor of the GitHub storage layout, introduction of legacy migration logic, and changes to core installation and validation paths require human review.

@amondnet amondnet self-assigned this Apr 12, 2026
@amondnet amondnet merged commit 88cdcdf into main Apr 12, 2026
4 of 5 checks passed
This was referenced Apr 12, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:XXL This PR changes 1000+ lines, ignoring generated files. status/draft Draft status:proposal Proposal for discussion type:docs Documentation improvements

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant