Add agnostic source resolution for private template families#19
Closed
next-devin wants to merge 6 commits into
Closed
Add agnostic source resolution for private template families#19next-devin wants to merge 6 commits into
next-devin wants to merge 6 commits into
Conversation
campaign-init fetched starter-template source from a single hardcoded public repo, so it could not scaffold private template families. - Add lib/template-source-registry.js: the single source of truth mapping a family slug to its source repo/ref/path. Public families resolve to the public starter-templates repo (anonymous); private families (e.g. arjuna → the Adsbranded private template repo) resolve to a private repo and are marked private. - init.js resolves repo/templates/campaigns/tarball URLs per family via the registry. Private families are fetched over an authenticated GitHub tarball (GITHUB_TOKEN/GH_TOKEN or `gh auth token`); the token is dropped on the signed codeload redirect so it never leaks cross-origin. Public families stay anonymous. A private family requested without a token fails with a clear, actionable error. - Private families are intentionally absent from any public templates.json, so they never surface in the picker; they resolve only by explicit slug. Verified end-to-end: arjuna pulls 210 files (pages + _includes + _layouts + assets) authenticated; olympus still resolves anonymously; missing token yields MISSING_GITHUB_TOKEN. Full test suite green (201 tests). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Overview
The previous WARNING (
The trust-boundary contract ("the resolver validates shape; the consumer can trust it") is now consistently POSIX end-to-end. Files Reviewed (3 files)
Previous Review Summaries (5 snapshots, latest commit 92e7a6b)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 92e7a6b)Status: 1 Issue Found | Recommendation: Address before merge Overview
The trust-boundary validation introduced in commit One cross-platform consistency gap remains and is flagged inline below. Issue Details (click to expand)CRITICAL(none) WARNING
SUGGESTION(none) Other Observations (not in diff)None — every pre-existing concern in Files Reviewed (4 files)
Fix these issues in Kilo Cloud Previous review (commit bf53ba7)Status: 2 Issues Found | Recommendation: Address before merge Overview
The refactor successfully removes per-family knowledge from page-kit: The two issues below are both about the new trust boundary: caller-supplied Issue Details (click to expand)WARNING
Files Reviewed (5 files)
Fix these issues in Kilo Cloud Previous review (commit e8ed87f)Status: No Issues Found | Recommendation: Merge Overview
Both previously flagged issues on this branch are now resolved:
Files Reviewed (2 files)
Previous review (commit a7ad921)Status: 2 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)SUGGESTION
Previously Reported — Resolved in a7ad921 (5)
Previously Reported — Intentionally Kept (1)
Files Reviewed (3 files)
Fix these issues in Kilo Cloud Previous review (commit 134b6bd)Status: 6 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Other Observations (not in diff)Issues found in unchanged code that cannot receive inline comments:
Files Reviewed (3 files)
Reviewed by minimax-m3 · 150,943 tokens |
Kilo Code review on #19: - init.js: missing GitHub token now exits 5 (MISSING_INPUT) instead of 4 (UPSTREAM_FETCH_FAILED) — no fetch was attempted, it's a missing precondition; the MISSING_GITHUB_TOKEN code string stays so callers can tell it apart from a missing --template. - registry+init.js: thread source_path through extractTemplate (default 'src') so the registry field is honored — a future family with a non-src/ layout resolves instead of silently failing. Was declared but unused. - registry: rename tarball_url -> tarball_url_authenticated so the authed (api.github.com) vs anonymous (codeload) paths are unambiguous and a caller can't accidentally make an authenticated request for a public family. - init.js: a private family's synthetic descriptor defaulted its display name to the slug; now upgraded to the private repo's registry name after the campaigns fetch when no --name was passed (verified: --template arjuna -> "Arjuna", slug unchanged). Removes the over-promising comment. - Hardening: fetchBufferAuthed refuses non-https redirects; note that the token must never be serialized into result/JSON/spinner output. 201 tests green; verified live (private fetch extracts 210 files, no-token exits 5). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Kilo Code review (round 3) on #19: - init.js: removed the "defense in depth" re-resolve in the public picker branch. A slug from the public manifest always resolves to the public source, so the re-resolve could only ever (in a misconfiguration) flip `source` to private while `token` stayed null — leading to a `Bearer null` fetch. Replaced with an explicit invariant: if a public-manifest slug resolves private, fail loudly (PRIVATE_FAMILY_IN_PUBLIC_MANIFEST) instead of attempting an unauthenticated private fetch. - init.js: fetchBufferAuthed now pins redirect targets to https AND a GitHub-owned host (github.com / *.github.com / *.githubusercontent.com) via a new isTrustedGithubRedirect() helper, not just the https scheme. Hardens the authed tarball path against a compromised/malicious upstream redirector. Unit tested (allows codeload/api/objects/github.com; rejects downgrade, foreign hosts, and suffix look-alikes like github.com.attacker.io). 203 tests green; verified the real api.github.com -> codeload redirect still extracts the private tarball. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Refactor per review: page-kit should be a generic build tool, not carry a hardcoded map of which family lives in which (private) repo. - Replace lib/template-source-registry.js (which hardcoded a family→repo map, incl. a specific private family) with lib/template-source.js: a pure resolver `resolveTemplateSource(overrides)` over the public starter-templates default. No family names, no private-repo names — page-kit holds zero per-family source knowledge. - init.js: add --source-repo / --source-ref / --source-path / --private flags. The caller (operator, or an orchestrator like campaigns-os) supplies the source; --private drives the authenticated, unlisted fetch path. Privateness is an explicit caller decision, never inferred from a slug. - Remove the now-moot family-map artifacts: the picker re-resolve and the PRIVATE_FAMILY_IN_PUBLIC_MANIFEST invariant (a public-branch source can no longer be private), and isPrivateFamily/FAMILY_SOURCES. - Keep all the prior mechanism hardening (exit 5 for missing token, source_path threading, authed-redirect host pin, https-only redirects, name refinement). 204 tests green. Verified: `--template arjuna --source-repo <repo> --private` extracts 210 files (name refined from campaigns.json); --private without a token exits 5; public `--template olympus` still resolves anonymously with no flags. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
next-devin
changed the title
Review (kilo-code-bot, 2 WARNINGs): the new --source-repo/--source-ref/ --source-path flags flowed into URL construction and tarball extraction with no shape validation. Notably --source-path "../../" could let extractTemplate path.join its way out of the extracted tarball subtree (then fs.cpSync it). resolveTemplateSource now validates shape (defense in depth — covers programmatic callers, not just the CLI) and throws INVALID_SOURCE on: - repo not matching strict "owner/repo" (rejects traversal, query strings, odd chars) - ref with ".."/leading/trailing slash/odd chars (slashed refs like feature/foo and refs/heads/x still allowed) - source_path that is absolute, "~", or contains ".." (relative subtree only) init.js surfaces the throw as a clean EXIT.INVALID_INPUT (6) / INVALID_SOURCE instead of a confusing upstream-fetch error. 208 tests green; verified a bad --source-path exits 6 and the real private source still validates + extracts. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Review (kilo-code-bot, WARNING): the validator normalized source_path with path.posix but extractTemplate joined it with native path.join, so on Windows a backslash source_path would pass POSIX validation yet be reinterpreted as a separator by path.win32.join — same input, different result per platform. - template-source.js: reject backslashes in source_path so it is strictly a POSIX subtree (forward slashes only) on every platform. - init.js extractTemplate: build the source-relative subtree with path.posix.join (matches the validator's semantics), then join onto the native tmp dir for the real filesystem path. The trust-boundary contract is now platform-independent. (Also removed a stray NUL byte that had crept into a source_path test fixture.) 208 tests green, incl. backslash-rejection cases. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Author
|
Closing in favor of #20, which carries the identical net change as a single clean commit. #19's history ended up build-then-refactor (a hardcoded-family registry, then refactored agnostic) across several review rounds; #20 presents only the final source-agnostic design. All review feedback from this PR is incorporated there. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
campaign-initpreviously fetched starter-template source from one hardcoded public repo, so it couldn't scaffold template families hosted elsewhere (e.g. an outside, access-controlled repo). This adds a source-agnostic resolver + authenticated fetch so any repo/ref/subtree can be a source — while keeping page-kit a generic build tool with zero per-family knowledge.Design (agnostic by construction)
lib/template-source.js(new) —resolveTemplateSource(overrides)overlays caller-supplied{repo, ref, source_path, private}on the public starter-templates default. It names no families and no private repos. Privateness is an explicit caller decision, never inferred from a slug.lib/actions/init.js— new flags:--source-repo,--source-ref,--source-path,--private.--private: authenticated tarball fetch (GITHUB_TOKEN/GH_TOKENorgh auth token); skips the public picker; requires--template. Token is dropped on the signed redirect, which is pinned to https + a GitHub-owned host.MISSING_INPUT, codeMISSING_GITHUB_TOKEN); no fetch attempted.--privatesource's display name is refined from the source repo'scampaigns.jsonwhen--nameis omitted.The opinionated part — which family lives in which private repo — stays in the orchestrating layer (campaigns-os / the operator's invocation), not in page-kit.
Verification
--template <slug> --source-repo <owner/repo> --private→ authenticated fetch, files extracted, name refined fromcampaigns.json.--privatewithout a token → exits 5.--template olympuswith no source flags → resolves anonymously (unchanged).Pairs with campaigns-os#101 (the Arjuna recognition contracts — repo-agnostic).