Skip to content

Add bear race mechanic#36

Open
truevox wants to merge 2 commits intomainfrom
claude/install-gstack-FNORM
Open

Add bear race mechanic#36
truevox wants to merge 2 commits intomainfrom
claude/install-gstack-FNORM

Conversation

@truevox
Copy link
Copy Markdown
Owner

@truevox truevox commented Mar 26, 2026

Bears that were already living on the mountain now become race opponents the moment the player starts
sledding downhill. They steer toward the player to jostle and bump them (bumpercars-style), creating a
natural cascade of competition as the player descends past more bears.

What's in this PR:

  • Bumpercars AI — racing bears accumulate gravity and steer laterally toward the player's X position,
    applying a bump impulse on close contact
  • Continuous enrollment — bears within 1 screen height above and 2 screen widths laterally auto-join
    the race each frame as the player descends; no one-time trigger
  • Bear radar — screen-edge arrow indicators point at off-screen racing bears, fading in as they get
    close
  • Force-spawn guarantee — if 3 seconds pass with no racing bear nearby, one spawns just above the
    visible screen so the player always has competition
  • Race result moment — win/lose/mixed floating text with a $100/bear money bonus, color-coded green
    (win) / gold (mixed) / brown (loss)
  • Scatter animation — on race end, bears dash off screen before snapping back to their home layer
  • Stamina fix — game now properly starts in UPHILL mode; reaching the peak auto-transitions to
    DOWNHILL; Tab still works anywhere as a manual toggle

claude and others added 2 commits March 24, 2026 22:10
Adds Garry Tan's gstack open-source engineering toolkit as a project skill
collection under .claude/skills/gstack, enabling slash commands like
/office-hours, /review, /qa, /ship, and more. Creates CLAUDE.md documenting
the available skills.

https://claude.ai/code/session_01RFRc5VXgSqsuBZGki1aaQG
When the player enters DOWNHILL, nearby bears (within 1 screen height
above, 2 screen widths laterally) automatically join the race and steer
toward the player to jostle them — bumpercars style. Bears continuously
enroll as the player descends past their altitude.

Race features:
- Bumpercars AI: gravity + lateral steering toward player X, bump impulse
  on collision
- Bear radar: screen-edge arrow indicators pointing at off-screen racing
  bears, fading in as they get closer
- Force-spawn guarantee: if 3 seconds pass with no racing bear nearby,
  one spawns just above the visible screen so the player always has
  competition
- Race result moment: win/lose/mixed floating text with money bonus ($100
  per bear beaten), color-coded green/gold/brown
- Scatter animation: on race end, bears dash off screen at speed before
  snapping back to their home layer — no instant teleport
- Stamina fix: game now properly starts in UPHILL mode; reaching the peak
  auto-transitions to DOWNHILL; Tab still works anywhere as manual toggle

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@netlify
Copy link
Copy Markdown

netlify bot commented Mar 26, 2026

Deploy Preview for sledhead ready!

Name Link
🔨 Latest commit 936e269
🔍 Latest deploy log https://app.netlify.com/projects/sledhead/deploys/69c49ef58937f60008603a68
😎 Deploy Preview https://deploy-preview-36--sledhead.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

@truevox truevox requested a review from Copilot March 26, 2026 02:51
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR vendors and expands the gstack skill set and its browse (Playwright-backed) headless browser daemon, adding new skills/docs plus substantial new functionality and test coverage around navigation security, cookie importing, snapshotting, and Windows compatibility.

Changes:

  • Add new skills/templates (e.g., /careful, /canary, /benchmark, /browse) and generated docs.
  • Implement/extend browse server + command set (URL validation, path validation, cookie picker routes, snapshot flags, buffers, Windows Node fallback polyfill).
  • Add a large Bun test suite with fixtures to validate the new browse behaviors end-to-end.

Reviewed changes

Copilot reviewed 77 out of 235 changed files in this pull request and generated 8 comments.

Show a summary per file
File Description
.claude/skills/gstack/careful/SKILL.md.tmpl Adds /careful skill template with destructive-command guardrails
.claude/skills/gstack/careful/SKILL.md Generated /careful skill doc from template
.claude/skills/gstack/canary/SKILL.md.tmpl Adds /canary post-deploy monitoring skill template
.claude/skills/gstack/browse/test/url-validation.test.ts Adds tests for dangerous scheme + metadata endpoint blocking
.claude/skills/gstack/browse/test/test-server.ts Adds tiny Bun test server for HTML fixtures
.claude/skills/gstack/browse/test/platform.test.ts Adds tests for platform constants + isPathWithin
.claude/skills/gstack/browse/test/path-validation.test.ts Adds tests for output/read path validation helpers
.claude/skills/gstack/browse/test/handoff.test.ts Adds tests for handoff/resume and state persistence
.claude/skills/gstack/browse/test/gstack-config.test.ts Adds tests for bin/gstack-config script behavior
.claude/skills/gstack/browse/test/fixtures/upload.html Adds upload fixture page
.claude/skills/gstack/browse/test/fixtures/states.html Adds element-states fixture page
.claude/skills/gstack/browse/test/fixtures/spa.html Adds SPA fixture with console output
.claude/skills/gstack/browse/test/fixtures/snapshot.html Adds snapshot fixture with mixed interactive elements
.claude/skills/gstack/browse/test/fixtures/responsive.html Adds responsive layout fixture page
.claude/skills/gstack/browse/test/fixtures/qa-eval.html Adds QA-eval fixture with intentionally planted bugs
.claude/skills/gstack/browse/test/fixtures/qa-eval-spa.html Adds SPA QA-eval fixture with intentionally planted bugs
.claude/skills/gstack/browse/test/fixtures/qa-eval-checkout.html Adds checkout QA-eval fixture with intentionally planted bugs
.claude/skills/gstack/browse/test/fixtures/forms.html Adds forms fixture page
.claude/skills/gstack/browse/test/fixtures/empty.html Adds minimal empty-page fixture
.claude/skills/gstack/browse/test/fixtures/dialog.html Adds dialog fixture page
.claude/skills/gstack/browse/test/fixtures/cursor-interactive.html Adds cursor-interactive fixture page
.claude/skills/gstack/browse/test/fixtures/basic.html Adds basic content fixture page
.claude/skills/gstack/browse/test/find-browse.test.ts Adds tests for locating the compiled browse binary
.claude/skills/gstack/browse/test/cookie-picker-routes.test.ts Adds route-level tests for cookie picker JSON/CORS behavior
.claude/skills/gstack/browse/test/config.test.ts Adds tests for config resolution, gitignore updates, health checks, etc.
.claude/skills/gstack/browse/test/bun-polyfill.test.ts Adds tests for Node Bun polyfill behavior
.claude/skills/gstack/browse/src/write-commands.ts Adds/extends write commands (click routing, upload, cookie import, cookie picker open)
.claude/skills/gstack/browse/src/url-validation.ts Adds URL validation (scheme blocking + metadata endpoint protection + DNS rebinding guard)
.claude/skills/gstack/browse/src/snapshot.ts Implements snapshot flags parsing, cursor-interactive scan, annotate/diff support
.claude/skills/gstack/browse/src/server.ts Adds/extends browse server (auth, buffers flush, help text, health route, cookie picker routing)
.claude/skills/gstack/browse/src/read-commands.ts Adds/extends read commands (path validation, is state checks, secret redaction)
.claude/skills/gstack/browse/src/platform.ts Adds cross-platform TEMP_DIR + path-within helper
.claude/skills/gstack/browse/src/meta-commands.ts Adds meta commands (screenshot/pdf/responsive, chain, diff, handoff/resume) + output path validation
.claude/skills/gstack/browse/src/find-browse.ts Adds binary discovery tool for browse executable
.claude/skills/gstack/browse/src/cookie-picker-routes.ts Adds cookie picker HTTP route handler (UI + JSON API)
.claude/skills/gstack/browse/src/config.ts Adds shared config resolution + .gstack/ state dir + .gitignore integration
.claude/skills/gstack/browse/src/commands.ts Adds central command registry + descriptions with load-time validation
.claude/skills/gstack/browse/src/bun-polyfill.cjs Adds Node polyfill for Bun APIs (Windows fallback)
.claude/skills/gstack/browse/src/buffers.ts Adds circular buffers + shared logging entry types
.claude/skills/gstack/browse/scripts/build-node-server.sh Adds Node-compatible server bundling script for Windows
.claude/skills/gstack/browse/SKILL.md.tmpl Adds /browse skill template docs for commands and workflows
.claude/skills/gstack/benchmark/SKILL.md.tmpl Adds /benchmark performance regression workflow template
.claude/skills/gstack/agents/openai.yaml Adds agent interface metadata for gstack bundle
.claude/skills/gstack/actionlint.yaml Adds actionlint config (runner label)
.claude/skills/gstack/VERSION Adds gstack version file
.claude/skills/gstack/SKILL.md.tmpl Adds top-level gstack skill template
.claude/skills/gstack/README.md Adds gstack README (installation, workflows, tooling overview)
.claude/skills/gstack/LICENSE Adds MIT license
.claude/skills/gstack/ETHOS.md Adds builder ethos documentation
.claude/skills/gstack/CLAUDE.md Adds contributor/dev workflow documentation
.claude/skills/gstack/BROWSER.md Adds browser internals + command reference documentation
.claude/skills/gstack/AGENTS.md Adds skills overview for non-Claude hosts
.claude/skills/gstack/.gitignore Adds gstack-local ignore rules (dist, logs, state dirs, etc.)
.claude/skills/gstack/.github/workflows/skill-docs.yml Adds CI check to enforce generated skill docs freshness
.claude/skills/gstack/.github/workflows/evals.yml Adds CI E2E eval workflow with docker caching + PR reporting
.claude/skills/gstack/.github/workflows/ci-image.yml Adds workflow to build/push CI runner image
.claude/skills/gstack/.github/workflows/actionlint.yml Adds workflow linting job via actionlint
.claude/skills/gstack/.github/docker/Dockerfile.ci Adds CI docker image definition with Bun/Node/Playwright/Claude CLI
.claude/skills/gstack/.github/actionlint.yaml Adds self-hosted runner label config for actionlint
.claude/skills/gstack/.env.example Adds example env file for evals

Comment on lines +1 to +5
---
name: careful
version: 0.1.0
description: |
MANUAL TRIGGER ONLY: invoke only when user types /careful.
Copy link

Copilot AI Mar 26, 2026

Choose a reason for hiding this comment

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

The PR title/description is about a "bear race mechanic", but the actual changes vendor the gstack skill suite and implement/extend a Playwright-based browse daemon (+ skills/docs/CI). Please update either the PR title/description to reflect the gstack/browse work, or split these changes into the correct PR(s).

Copilot uses AI. Check for mistakes.
Comment on lines +238 to +245
case 'upload': {
const [selector, ...filePaths] = args;
if (!selector || filePaths.length === 0) throw new Error('Usage: browse upload <selector> <file1> [file2...]');

// Validate all files exist before upload
for (const fp of filePaths) {
if (!fs.existsSync(fp)) throw new Error(`File not found: ${fp}`);
}
Copy link

Copilot AI Mar 26, 2026

Choose a reason for hiding this comment

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

The new upload command can read arbitrary local files and upload them to any page, which is a high-risk exfiltration vector for an agent-capable tool. Consider restricting uploads to the same safe directories used elsewhere (TEMP_DIR, process.cwd()), and reject traversal (e.g., ../) similarly to validateReadPath/validateOutputPath.

Copilot uses AI. Check for mistakes.
Comment on lines +50 to +53
const normalized = path.normalize(filePath);
if (normalized.includes('..')) {
throw new Error('Path traversal sequences (..) are not allowed');
}
Copy link

Copilot AI Mar 26, 2026

Choose a reason for hiding this comment

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

This traversal check will also block legitimate paths that merely contain .. in a filename (e.g., notes..backup.js). To avoid false positives (and also handle both separators), check for .. as a path segment (e.g., split by path.sep after normalization, or use a regex like /(^|[\\\\/])\\.\\.([\\\\/]|$)/). The same pattern appears elsewhere (e.g., cookie import).

Copilot uses AI. Check for mistakes.

/** Check if resolvedPath is within dir, using platform-aware separators. */
export function isPathWithin(resolvedPath: string, dir: string): boolean {
return resolvedPath === dir || resolvedPath.startsWith(dir + path.sep);
Copy link

Copilot AI Mar 26, 2026

Choose a reason for hiding this comment

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

On Windows, path comparisons are case-insensitive and may contain mixed separators, so this string-prefix check can incorrectly reject valid paths (or behave inconsistently) if casing differs (e.g., C:\\Temp vs c:\\temp). A more robust approach is to normalize/resolve both sides (and on Windows, compare with a consistent casing) before checking containment.

Suggested change
return resolvedPath === dir || resolvedPath.startsWith(dir + path.sep);
const normalizedPath = path.resolve(resolvedPath);
const normalizedDir = path.resolve(dir);
const pathToCompare = IS_WINDOWS ? normalizedPath.toLowerCase() : normalizedPath;
const dirToCompare = IS_WINDOWS ? normalizedDir.toLowerCase() : normalizedDir;
const dirWithSep = dirToCompare.endsWith(path.sep)
? dirToCompare
: dirToCompare + path.sep;
return pathToCompare === dirToCompare || pathToCompare.startsWith(dirWithSep);

Copilot uses AI. Check for mistakes.
head -1 "$DIST_DIR/server-node.mjs"
echo '// ── Windows Node.js compatibility (auto-generated) ──'
echo 'import { fileURLToPath as _ftp } from "node:url";'
echo 'import { dirname as _dn } from "node:path";'
Copy link

Copilot AI Mar 26, 2026

Choose a reason for hiding this comment

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

The injected header uses createRequire(...) but doesn’t import it, which will throw at runtime in Node ESM (ReferenceError: createRequire is not defined). Import it explicitly (e.g., from node:module) in the injected header so the generated server-node.mjs is self-contained.

Suggested change
echo 'import { dirname as _dn } from "node:path";'
echo 'import { dirname as _dn } from "node:path";'
echo 'import { createRequire } from "node:module";'

Copilot uses AI. Check for mistakes.
Comment on lines +312 to +318
const screenshotPath = opts.outputPath || `${TEMP_DIR}/browse-annotated.png`;
// Validate output path (consistent with screenshot/pdf/responsive)
const resolvedPath = require('path').resolve(screenshotPath);
const safeDirs = [TEMP_DIR, process.cwd()];
if (!safeDirs.some((dir: string) => isPathWithin(resolvedPath, dir))) {
throw new Error(`Path must be within: ${safeDirs.join(', ')}`);
}
Copy link

Copilot AI Mar 26, 2026

Choose a reason for hiding this comment

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

snapshot.ts is TypeScript/ESM, but this block uses require('path'). That will break when running in a pure ESM Node bundle (and is also inconsistent with other modules that import * as path from 'path'). Prefer a static import at the top and use path.resolve(...) here.

Copilot uses AI. Check for mistakes.
Comment on lines +26 to +28
afterAll(() => {
try { testServer.server.stop(); } catch {}
setTimeout(() => process.exit(0), 500);
Copy link

Copilot AI Mar 26, 2026

Choose a reason for hiding this comment

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

Calling process.exit(0) from afterAll will terminate the entire Bun test process and can prevent other test files from running/finishing when the full suite is executed. Replace this with explicit resource cleanup (await bm.close(), ensure all Playwright instances are closed, stop the server), and let the test runner exit naturally.

Suggested change
afterAll(() => {
try { testServer.server.stop(); } catch {}
setTimeout(() => process.exit(0), 500);
afterAll(async () => {
try {
if (bm) {
await bm.close();
}
} catch {}
try {
testServer.server.stop();
} catch {}

Copilot uses AI. Check for mistakes.
test('priority chain checks .codex, .agents, .claude markers', () => {
// Verify the source code implements the correct priority order.
// We read the function source to confirm the markers array order.
const src = require('fs').readFileSync(require('path').join(__dirname, '../src/find-browse.ts'), 'utf-8');
Copy link

Copilot AI Mar 26, 2026

Choose a reason for hiding this comment

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

This test mixes CommonJS globals (require, __dirname) inside an ESM-style test file (import { describe, test, expect } ...). In many Bun/TS ESM contexts, __dirname (and sometimes require) is undefined, causing a runtime crash. Use import.meta.dir for the directory and regular import (or await import(...)) patterns instead.

Copilot uses AI. Check for mistakes.
@gemini-code-assist
Copy link
Copy Markdown
Contributor

Summary of Changes

Hello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request introduces a comprehensive bear racing system, transforming passive mountain bears into active competitors. The new mechanics aim to enhance player engagement by providing dynamic challenges and clear feedback on race outcomes, alongside crucial fixes to game mode transitions.

Highlights

  • New Game Mechanic: Bear Race: Introduced a dynamic bear race mechanic where existing mountain bears become opponents, actively steering towards the player to jostle and bump them during downhill sledding.
  • Intelligent Opponent Behavior: Implemented 'Bumpercars AI' for racing bears, allowing them to accumulate gravity, steer laterally towards the player's X position, and apply bump impulses on close contact.
  • Continuous Race Enrollment & Spawning: Bears within a defined screen area continuously join the race as the player descends, and a 'force-spawn guarantee' ensures new opponents appear if no racing bears are nearby for 3 seconds.
  • Enhanced Player Awareness: Added 'Bear radar' with screen-edge arrow indicators that point to off-screen racing bears, fading in as they approach the visible area.
  • Race Outcome Feedback: Integrated a 'Race result moment' feature that displays win/lose/mixed floating text, provides a $100 money bonus per bear, and uses color-coded feedback (green for win, gold for mixed, brown for loss).
  • Post-Race Animations: Included a 'Scatter animation' where bears dash off-screen at the end of a race before returning to their home layer.
  • Stamina System Refinement: Corrected the game's stamina system to properly start in UPHILL mode, auto-transition to DOWNHILL upon reaching the peak, and maintain manual toggle functionality via the Tab key.
Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help /gemini help Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here.

Footnotes

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution.

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request introduces significant infrastructure and documentation updates for the gstack project. Key changes include new GitHub Actions for workflow linting, CI image building, and E2E evaluations, alongside a refined Dockerfile for the CI environment. Extensive documentation has been added or updated, covering agent skills, architecture, browser reference, development guidelines, and project ethos. New utility scripts for development setup, telemetry, and git repository analysis have also been introduced or modified. Review feedback highlights several areas for improvement, including addressing an insecure chmod command in the Dockerfile, consolidating apt-get update commands for efficiency, improving shell script portability, removing a duplicated .env entry, correcting telemetry processing logic that prematurely breaks loops, using jq for robust JSON parsing, and eliminating redundant slug generation scripts.

&& mkdir -p /home/runner/.gstack && chown -R runner:runner /home/runner/.gstack \
&& chmod 1777 /tmp \
&& mkdir -p /home/runner/.bun && chown -R runner:runner /home/runner/.bun \
&& chmod -R 1777 /tmp
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.

high

The command chmod -R 1777 /tmp is insecure as it makes all files and subdirectories in /tmp world-writable, which can lead to security vulnerabilities. The correct permission for the /tmp directory itself is 1777 (with the sticky bit), which is already correctly set on line 61. This recursive chmod is unnecessary and should be removed to improve security.

Comment on lines +8 to +23
RUN apt-get update && apt-get install -y --no-install-recommends \
git curl unzip ca-certificates jq bc gpg \
&& rm -rf /var/lib/apt/lists/*

# GitHub CLI
RUN curl -fsSL https://cli.github.com/packages/githubcli-archive-keyring.gpg \
| gpg --dearmor -o /usr/share/keyrings/githubcli-archive-keyring.gpg \
&& echo "deb [arch=$(dpkg --print-architecture) signed-by=/usr/share/keyrings/githubcli-archive-keyring.gpg] https://cli.github.com/packages stable main" \
| tee /etc/apt/sources.list.d/github-cli.list > /dev/null \
&& apt-get update && apt-get install -y --no-install-recommends gh \
&& rm -rf /var/lib/apt/lists/*

# Node.js 22 LTS (needed for claude CLI)
RUN curl -fsSL https://deb.nodesource.com/setup_22.x | bash - \
&& apt-get install -y --no-install-recommends nodejs \
&& rm -rf /var/lib/apt/lists/*
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.

medium

The Dockerfile has multiple RUN apt-get update && ... && rm -rf /var/lib/apt/lists/* commands. This is inefficient as it creates multiple layers and runs apt-get update repeatedly. It's a best practice to combine these into a single RUN instruction to reduce image size and build time. Consider performing all package source setup first, then a single apt-get update, followed by a single apt-get install for all packages, and finally a single cleanup step.


| Suite | Result | Status | Cost |
|-------|--------|--------|------|
$(echo -e "$SUITE_LINES")
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.

medium

Using echo -e can have inconsistent behavior across different shells and systems. For more portable and safer interpretation of backslash escapes like \n, it's recommended to use printf. This will ensure the markdown table for the PR comment is always formatted correctly.

          $(printf '%b' "$SUITE_LINES")

*.log
bun.lock
*.bun-build
.env
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.

medium

The entry .env is duplicated. It's already present on line 1. You can remove this redundant entry.

mkdir -p ~/.gstack/analytics
echo '{"skill":"gstack","ts":"'$(date -u +%Y-%m-%dT%H:%M:%SZ)'","repo":"'$(basename "$(git rev-parse --show-toplevel 2>/dev/null)" 2>/dev/null || echo "unknown")'"}' >> ~/.gstack/analytics/skill-usage.jsonl 2>/dev/null || true
# zsh-compatible: use find instead of glob to avoid NOMATCH error
for _PF in $(find ~/.gstack/analytics -maxdepth 1 -name '.pending-*' 2>/dev/null); do [ -f "$_PF" ] && ~/.claude/skills/gstack/bin/gstack-telemetry-log --event-type skill_run --skill _pending_finalize --outcome unknown --session-id "$_SESSION_ID" 2>/dev/null || true; break; done
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.

medium

The loop to finalize stale pending telemetry events has a break statement, which means it will only process one stale file per run, even if multiple exist. This could lead to a backlog of unprocessed pending events. To ensure all stale files are processed, the break should be removed.

Suggested change
for _PF in $(find ~/.gstack/analytics -maxdepth 1 -name '.pending-*' 2>/dev/null); do [ -f "$_PF" ] && ~/.claude/skills/gstack/bin/gstack-telemetry-log --event-type skill_run --skill _pending_finalize --outcome unknown --session-id "$_SESSION_ID" 2>/dev/null || true; break; done
for _PF in $(find ~/.gstack/analytics -maxdepth 1 -name '.pending-*' 2>/dev/null); do [ -f "$_PF" ] && ~/.claude/skills/gstack/bin/gstack-telemetry-log --event-type skill_run --skill _pending_finalize --outcome unknown --session-id "$_SESSION_ID" 2>/dev/null || true; done

Comment on lines +44 to +45
WEEKLY="$(echo "$DATA" | grep -o '"weekly_active":[0-9]*' | grep -o '[0-9]*' || echo "0")"
CHANGE="$(echo "$DATA" | grep -o '"change_pct":[0-9-]*' | grep -o '[0-9-]*' || echo "0")"
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.

medium

Parsing JSON with grep and awk is fragile and can break if the JSON formatting changes (e.g., with extra whitespace). Since jq is installed as part of the environment (as seen in the Dockerfile), it would be more robust to use jq for parsing the JSON response from the Supabase edge function. This applies to other JSON parsing in this script as well.

WEEKLY="$(echo "$DATA" | jq -r '.weekly_active // 0')"
CHANGE="$(echo "$DATA" | jq -r '.change_pct // 0')"

Comment on lines +16 to +22
REMOTE_URL=$(git remote get-url origin 2>/dev/null || true)
if [ -z "$REMOTE_URL" ]; then
echo "REPO_MODE=unknown"
exit 0
fi
SLUG=$(echo "$REMOTE_URL" | sed 's|.*[:/]\([^/]*/[^/]*\)\.git$|\1|;s|.*[:/]\([^/]*/[^/]*\)$|\1|' | tr '/' '-')
[ -z "${SLUG:-}" ] && { echo "REPO_MODE=unknown"; exit 0; }
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.

medium

This script re-implements the logic for getting the repository slug, which is also present in the gstack-slug script. To avoid duplication and ensure consistency (e.g., sanitization of the slug), you could call gstack-slug from this script. The main gstack-slug script also includes sanitization which makes it more robust.

eval "$($SCRIPT_DIR/gstack-slug 2>/dev/null)"
[ -z "${SLUG:-}" ] && { echo "REPO_MODE=unknown"; exit 0; }

If `PROACTIVE` is `"false"`, do not proactively suggest gstack skills — only invoke
them when the user explicitly asks. The user opted out of proactive suggestions.

If output shows `UPGRADE_AVAILABLE <old> <new>`: read `~/.claude/skills/gstack/gstack-upgrade/SKILL.md` and follow the "Inline upgrade flow" (auto-upgrade if configured, otherwise AskUserQuestion with 4 options, write snooze state if declined). If `JUST_UPGRADED <from> <to>`: tell user "Running gstack v{to} (just updated!)" and continue.
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.

medium

The loop to finalize stale pending telemetry events has a break statement, which means it will only process one stale file per run, even if multiple exist. This could lead to a backlog of unprocessed pending events. To ensure all stale files are processed, the break should be removed.

Suggested change
If output shows `UPGRADE_AVAILABLE <old> <new>`: read `~/.claude/skills/gstack/gstack-upgrade/SKILL.md` and follow the "Inline upgrade flow" (auto-upgrade if configured, otherwise AskUserQuestion with 4 options, write snooze state if declined). If `JUST_UPGRADED <from> <to>`: tell user "Running gstack v{to} (just updated!)" and continue.
for _PF in $(find ~/.gstack/analytics -maxdepth 1 -name '.pending-*' 2>/dev/null); do [ -f "$_PF" ] && ~/.claude/skills/gstack/bin/gstack-telemetry-log --event-type skill_run --skill _pending_finalize --outcome unknown --session-id "$_SESSION_ID" 2>/dev/null || true; done

Comment on lines +1 to +14
#!/usr/bin/env bash
# Output the remote slug (owner-repo) for the current git repo.
# Used by SKILL.md files to derive project-specific paths in ~/.gstack/projects/.
set -e
URL=$(git remote get-url origin 2>/dev/null || true)
if [ -n "$URL" ]; then
# Strip trailing .git if present, then extract owner/repo
URL="${URL%.git}"
# Handle both SSH (git@host:owner/repo) and HTTPS (https://host/owner/repo)
OWNER_REPO=$(echo "$URL" | sed -E 's#.*[:/]([^/]+)/([^/]+)$#\1-\2#')
echo "$OWNER_REPO"
else
basename "$(git rev-parse --show-toplevel 2>/dev/null || pwd)"
fi
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.

medium

This script duplicates functionality found in bin/gstack-slug. To reduce code duplication and ensure consistency, consider removing this script and using the more robust bin/gstack-slug script, which also includes output sanitization.

mkdir -p ~/.gstack/analytics
echo '{"skill":"canary","ts":"'$(date -u +%Y-%m-%dT%H:%M:%SZ)'","repo":"'$(basename "$(git rev-parse --show-toplevel 2>/dev/null)" 2>/dev/null || echo "unknown")'"}' >> ~/.gstack/analytics/skill-usage.jsonl 2>/dev/null || true
# zsh-compatible: use find instead of glob to avoid NOMATCH error
for _PF in $(find ~/.gstack/analytics -maxdepth 1 -name '.pending-*' 2>/dev/null); do [ -f "$_PF" ] && ~/.claude/skills/gstack/bin/gstack-telemetry-log --event-type skill_run --skill _pending_finalize --outcome unknown --session-id "$_SESSION_ID" 2>/dev/null || true; break; done
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.

medium

The loop to finalize stale pending telemetry events has a break statement, which means it will only process one stale file per run, even if multiple exist. This could lead to a backlog of unprocessed pending events. To ensure all stale files are processed, the break should be removed.

Suggested change
for _PF in $(find ~/.gstack/analytics -maxdepth 1 -name '.pending-*' 2>/dev/null); do [ -f "$_PF" ] && ~/.claude/skills/gstack/bin/gstack-telemetry-log --event-type skill_run --skill _pending_finalize --outcome unknown --session-id "$_SESSION_ID" 2>/dev/null || true; break; done
for _PF in $(find ~/.gstack/analytics -maxdepth 1 -name '.pending-*' 2>/dev/null); do [ -f "$_PF" ] && ~/.claude/skills/gstack/bin/gstack-telemetry-log --event-type skill_run --skill _pending_finalize --outcome unknown --session-id "$_SESSION_ID" 2>/dev/null || true; done

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants