* fix(runtime): pass auth environment variables to containers
The web container's envsubst in 30-runtime-config.sh only substituted
SKILLHUB_WEB_API_BASE_URL and SKILLHUB_PUBLIC_BASE_URL, leaving auth-related
variables (authDirectEnabled, authSessionBootstrapEnabled, etc.) as literal
${...} strings in runtime-config.js. Additionally, compose.release.yml did not
pass SKILLHUB_WEB_AUTH_DIRECT_ENABLED or SKILLHUB_WEB_AUTH_DIRECT_PROVIDER to
the web container, nor SKILLHUB_AUTH_DIRECT_ENABLED to the server container.
This made it impossible to enable direct (username/password) authentication
for intranet deployments without OAuth2, even though the frontend template and
backend already supported it.
Changes:
- compose.release.yml: add SKILLHUB_AUTH_DIRECT_ENABLED to server env
- compose.release.yml: add auth direct and session bootstrap vars to web env
- 30-runtime-config.sh: expand envsubst to cover all runtime-config.js template variables
- .env.release.example: document the new auth configuration variables
All new variables default to false/empty, preserving existing GitHub OAuth behavior.
* fix: remove session bootstrap frontend config from compose
Per reviewer feedback: exposing SKILLHUB_WEB_AUTH_SESSION_BOOTSTRAP_* in the
compose without matching SKILLHUB_AUTH_SESSION_BOOTSTRAP_ENABLED on the server
would cause 403 errors when frontend attempts bootstrap.
Keep this PR focused on direct auth only. Bootstrap variables are still handled
in 30-runtime-config.sh with false defaults, so runtime-config.js will have
authSessionBootstrapEnabled: "false" and frontend will not trigger bootstrap.
---------
Co-authored-by: wowo <zhenggui5228@126.com>
Co-authored-by: PR Review Helper <review-helper@local>
* fix(auth): use SimpleUrlAuthenticationSuccessHandler for OAuth2 login
Replace SavedRequestAwareAuthenticationSuccessHandler with
SimpleUrlAuthenticationSuccessHandler to prevent redirecting to
saved API requests after OAuth2 login.
Previously, when a user accessed a protected API endpoint (e.g.,
/api/web/skills) without authentication, Spring Security would save
that request. After OAuth2 login, the handler would redirect back to
the API endpoint instead of the dashboard.
Now the handler only uses:
- returnTo parameter from session (if present)
- default target URL (/dashboard) as fallback
* test(auth): add regression for OAuth2 success redirect; restore clearAuthenticationAttributes
Cover the no-returnTo + cached-API-request branch with HttpSessionRequestCache so
the original bug (post-login redirect resolving to /api/web/skills) cannot be
silently reintroduced. Also restore clearAuthenticationAttributes() in the
returnTo branch so it stays symmetric with the default branch (super clears it).
---------
Co-authored-by: xiose <huyanlin@nuaa.edu.cn>
Resolve historical naming drift between protocol spec and CLI by adopting the
plural form across both docs:
- docs/07-skill-protocol.md: drop the drift caveat; the four-tier priority is
now stated as .agents/skills / ~/.agents/skills / .claude/skills /
~/.claude/skills directly.
- docs/00-product-direction.md: align with the same plural form.
The CLI already uses .agents/skills (cli/src/agents/profiles/generic-fallback.ts
and cli/src/agents/resolver.ts). No code change required.
Covers the new "access denied — token may lack required scope" error
path with a fake-registry 'forbidden' failure mode. Prevents the
improved 403 message from regressing silently.
Disable automated PR review by Gemini Code Assist for GitHub by removing
.gemini/config.yaml. The repository will no longer trigger Gemini-based
PR summaries or review comments.
- Add ariaLabel prop to CopyButton for screen reader differentiation
- Pass per-user aria-label: "Copy user ID for {username}"
- Add truncation for long userIds (max-w-[14rem] + title tooltip)
- Scope E2E copy-button assertions to userId cell to avoid false positives
- Assert on span.font-mono for userId text to exclude button text
- Use toHaveText instead of getByRole name for "Copied" feedback check
- Fix test fixture: warnings-only response now uses valid=false to
match real backend behavior (warnings make dry-run invalid)
- Distinguish 403 from 401 in CLI error messages: 403 now says
"access denied — token may lack required scope" with a hint to
regenerate the token, rather than the generic "authentication failed"
- Fix race condition: use Promise.all for goto + waitForResponse
- Remove all waitForTimeout calls, use explicit assertions/waitForResponse
- Assert clipboard content equals the actual userId (not just non-empty)
- Fix unused variable (userIdText) that would fail lint --max-warnings 0
- Trigger real search via button click instead of just filling input
- Add status filter test to cover the filter path
- Add comment explaining mock-profile approach for admin session
ApiTokenAuthenticationFilter authenticates /api/cli/** Bearer tokens
but ApiTokenScopeFilter.shouldNotFilter() previously skipped them.
The result: API token requests on CLI routes were authenticated and
authorization-policy-checked, but scope enforcement never ran. Tokens
without skill:publish or skill:delete could call /publish, /publish/validate,
and DELETE despite the policy table requiring those scopes.
Add /api/cli/ to the scope filter's covered prefixes and a filter-level
test that confirms a token missing skill:publish is rejected on the new
validate endpoint. Update the existing CLI controller tests to grant
the appropriate SCOPE_* authorities to their api_token principals so
they continue to pass under enforced scopes.
Add a userId column with one-click copy functionality to the admin
user management table to help administrators easily access user IDs
for batch operations like namespace member management.
Changes:
- Add userId column after username in admin users table
- Implement one-click copy button for each userId
- Add i18n translations for column header (en/zh)
- Add comprehensive E2E tests (6 test cases)
Closes#426
Fix three blockers and one contract drift issue surfaced in code review:
1. API token policy: add skill:publish scope policy and authentication
policy for /api/cli/v1/skills/*/publish/validate. Without these the
AntPathMatcher pattern /publish would not cover /publish/validate,
so Bearer-token requests would be rejected by the scope filter.
2. Warnings semantics: dry-run now treats warnings as making valid=false.
The CLI publish flow uses confirmWarnings=false, so the real publish
rejects any warnings; dry-run must mirror that to avoid false positives.
3. Visibility parameter: validate endpoint now accepts the same
visibility multipart field as publish. The CLI forwards --visibility
so invalid values are caught at dry-run time rather than at publish.
4. Schema drift: resolvedSlug and resolvedVersion are nullable in
practice (returned as null when validation fails before resolution).
Updated schema.d.ts to reflect string | null instead of optional string.
Tests added:
- RouteSecurityPolicyRegistryTest: validate endpoint scope check
- CliDryRunValidateTest: custom + invalid visibility cases
- publish-dry-run.test.ts: --visibility forwarded to server
- Exit non-zero (code 6) when --dry-run validation fails, enabling
CI/CD pipeline integration
- Add archived skill check: dry-run now detects when the publisher's
own skill is archived
- Add version-exists check: dry-run now detects when the resolved
version is already published
- Use StandardCharsets.UTF_8 for SKILL.md content parsing
Add a validate-only endpoint (POST /api/cli/v1/skills/{namespace}/publish/validate)
that runs the full pre-publish validation chain without persisting anything.
This allows developers to check their package locally before actual publishing.
The validation covers:
- SKILL.md existence and frontmatter parsing (name, description required)
- File extension whitelist and size limits
- Credential leak scanning with line-number precision
- Slug generation and name conflict detection
CLI usage: `skillhub publish <path> --dry-run`
Closes#429
When --agent is provided without --scope, scope was inferred via
root.startsWith(cwd), which mislabels user roots as project when
cwd === home. Use profile.userRoots(home) membership instead, so the
candidate scope reflects the profile's intent rather than path prefix
overlap. The chosen root path itself is unchanged.
- Distinguish user vs project install scope via explicit --scope flag
- Interactive mode prompts for scope when --scope/--agent/--dir not provided
- Non-interactive bare install preserves existing behavior (backward compatible)
- Mutual exclusion: --dir cannot be combined with --scope or --agent
- Symmetric fallback: --scope user falls back to ~/.agents/skills,
--scope project falls back to <cwd>/.agents/skills
- Strict TTY check requires both stdin and stdout TTY plus no --json
- Scope-aware candidate generation avoids root.startsWith(cwd) misjudgement
when cwd === home or paths overlap
- Correct gemini-cli (.gemini/skills) and kiro-cli (.kiro/skills) paths
in install path tables across README and guide docs
- Note CLI fallback uses .agents/skills (with s) in skill protocol doc
When approving a promotion, the new SkillVersion was created without copying
bundleReady and downloadReady from the source version, causing the download
button to be permanently disabled for promoted skills.
Windows ZIP library produces backslashes in file paths while Unix uses forward slashes. Normalize all paths to forward slashes before assertion to ensure tests pass on all platforms.
- Fix Windows test failure by using regex that accepts both / and \ path separators in install-command.test.ts
- Rename contradictory test case in publish-command.test.ts from "surfaces a non-zero exit" to "is handled without crash" to match actual assertion behavior
Migrated 39 test files covering CLI integration and unit testing:
- 6 new integration tests (auth-resolution, concurrency, cross-command, inventory-resilience, multi-registry, version-upgrade-flow)
- Enhanced 7 existing integration tests with comprehensive scenarios
- Updated 2 unit tests with correct exit code expectations
All tests use fake registry approach (no E2E/browser required) and pass lint/build/test checks.
The `status="$(env ... printf | bash ... && echo 0 || echo $?)"` pattern
doesn't correctly capture the script's exit code because the command
substitution and pipe interact poorly. Use direct assignment with
`|| status=$?` instead.
The old approach (breaking origin URL) caused `git pull` to fail
before reaching the push step. Use a git wrapper that only fails
on `push` so the rest of the script runs normally.
Remove test 7 (remote tag race condition) — the scenario is nearly
impossible with the new baseline sync logic and too complex to
reliably simulate. Fix variable naming inconsistencies from the
renumbering.
Old test 7 used `--no-tags` config to prevent fetch from pulling the
remote tag, but that doesn't reflect any real-world scenario. With the
new baseline sync logic, a pre-existing remote tag would be synced
into the local version, eliminating the conflict path the test claimed
to cover.
Replace with a git wrapper that injects the conflicting tag into origin
right before the script's `ls-remote` check, which simulates a real
race between two developers attempting to release the same version.
Tests write stdout.log/stderr.log into the test repo root, which made
`git status --porcelain` non-empty and broke test 3 (non-main branch
abort) by tripping the dirty-tree check first.
Add a .gitignore to the test fixture repo to filter out these files.
publish-npm and create-release now checkout the same ref as
build-and-test (the input tag or push ref), preventing source
mismatch between npm package and GitHub Release artifacts.
1. npm version check: three-state logic (exists/missing/error) to prevent
silent skip on network failures, registry 5xx, or auth issues.
2. workflow_dispatch: checkout the specified tag and validate SHA matches,
preventing builds from wrong ref.
3. Atomic push: use `git push --atomic` and detect unpushed tags via
`git ls-remote` instead of `--no-merged` (catches branch-pushed-but-
tag-failed state).
Add pre-flight check in publish-cli.sh to detect unpushed commits and tags
from previous failed pushes. When detected, the script exits with clear
recovery instructions:
1. Retry push (for transient network failures)
2. Rollback and re-release (for clean restart)
This prevents the baseline sync logic from skipping failed versions when
local tags participate in version calculation after a push failure.
Addresses feedback from dongmucat in PR #422.
1. Update release-cli.yml to make create-release depend on publish-npm with proper skip_npm handling, preventing half-released state where GitHub Release exists but npm package is unavailable.
2. Rewrite publish-cli-test.sh to cover the new publish flow: main branch check, dirty tree detection, tag baseline sync, version bumping, tag conflict detection, user cancellation, and atomic push verification.
