Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
121 changes: 4 additions & 117 deletions .claude/learnings/pr366-self-hosted-runner-2025-12.md
Original file line number Diff line number Diff line change
Expand Up @@ -14,117 +14,10 @@ Migrated 5 GitHub Actions workflows from `ubuntu-latest` to self-hosted runners

### 1. GitHub Actions Permissions

**Pattern:** Always include explicit `permissions:` blocks in workflows.

**Fixed Files:**
- `.github/workflows/chit-contract.yml`
- `.github/workflows/python-tests.yml`
- `.github/workflows/webhook-smoke.yml`

**Change:**
```yaml
# After `on:` section, before `jobs:`:
permissions:
contents: read
```

**Why:** GitHub Actions defaults to `write-all` permissions for backward compatibility. Explicitly declaring `contents: read` follows the principle of least privilege and prevents workflows from accidentally having repository write access.

**Reference:** [GitHub Actions - Default permissions](https://docs.github.com/en/actions/security-guides/automatic-token-authentication#permissions-for-the-github_token)

---

## Out-of-Diff Fixes (Technical Debt)

### 2. Git Submodule Path Duplication

**File:** `.gitmodules`

**Problem:** Path duplication in e2b submodule:
```ini
[submodule "pmoves/pmoves/vendor/e2b"] # WRONG
path = pmoves/pmoves/vendor/e2b # WRONG
```

**Fixed:**
```ini
[submodule "pmoves/vendor/e2b"] # CORRECT
path = pmoves/vendor/e2b # CORRECT
```

**Why:** When adding submodules manually, it's easy to duplicate path components. The correct approach is to use `git submodule add` CLI which validates paths automatically.

**Lesson:** Never edit `.gitmodules` manually. Use:
```bash
git submodule add <url> <path>
```

---

### 3. Makefile Working Directory Context

**File:** `pmoves/Makefile`

**Problem:** Test-smoke targets used `cd pmoves && pytest ...`
```makefile
test-smoke:
cd pmoves && pytest tests/smoke/ -v -m smoke
```

**Why it fails:** When invoked via `make -C pmoves`, the working directory is already `pmoves/`. The `cd pmoves` tries to change into `pmoves/pmoves/` which doesn't exist.

**Fixed:**
```makefile
test-smoke:
pytest tests/smoke/ -v -m smoke
```

**Lesson:** When using `make -C <dir>`, never use `cd <dir>` in recipes. The working directory is already set to `<dir>`.

---

### 4. Makefile Undefined Variables

**File:** `pmoves/Makefile`

**Problem:** `$(SCRIPTS)` variable was used but not defined.

**Fixed:** Added at line 44:
```makefile
PYTHON ?= python3
SCRIPTS := scripts
SINGLE_ENV_MODE ?= 1
```

**Lesson:** Define all Makefile variables before use. Group related variable definitions together near the top of the file.

---

### 5. Makefile Standard Targets

**File:** `pmoves/Makefile`

**Problem:** Missing conventional `all` and `test` targets.

**Fixed:** Added after line 70:
```makefile
# -------- Standard Makefile targets ----------
.PHONY: all
all: help ## Default target - show help

.PHONY: test
test: test-smoke ## Run pytest smoke tests

.DEFAULT_GOAL := help
```

**Lesson:** POSIX Makefiles should include:
- `all` - default target (should show help)
- `test` - alias to project's test suite
- `clean` - remove build artifacts (already existed)
- `.DEFAULT_GOAL` - explicit default behavior

---
1. **GitHub Actions Permissions:** Always include explicit `permissions:` blocks for least privilege
2. **Git Submodules:** Use `git submodule add` CLI, never edit `.gitmodules` manually
3. **Makefile `make -C`:** Never use `cd` in recipes when using `make -C <dir>`
4. **Makefile Standards:** Include `all`, `test`, `clean` targets and `.DEFAULT_GOAL`

## Runner Labels Reference

Expand All @@ -133,8 +26,6 @@ test: test-smoke ## Run pytest smoke tests
| CPU (VPS) | `[self-hosted, vps]` | General CI, Python tests, contract checks |
| GPU (AI Lab) | `[self-hosted, ai-lab, gpu]` | ML models, CUDA workloads, TTS |

---

## Pre-Merge Checklist

- [x] All workflows have `permissions: contents: read`
Expand All @@ -144,8 +35,6 @@ test: test-smoke ## Run pytest smoke tests
- [ ] CI passes on self-hosted runners
- [ ] Smoke tests pass locally

---

## Commands for Validation

```bash
Expand All @@ -162,8 +51,6 @@ grep "^path=" .gitmodules | sort | uniq -d
cd pmoves && make test-smoke
```

---

## References

- [GitHub Actions Security](https://docs.github.com/en/actions/security-guides/security-hardening-for-github-actions)
Expand Down
3 changes: 3 additions & 0 deletions .github/workflows/chit-contract.yml
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,9 @@ on:
- 'pmoves/docs/SUPABASE_*.md'
- '.github/workflows/chit-contract.yml'

permissions:
contents: read

jobs:
verify:
runs-on: ubuntu-latest
Expand Down
6 changes: 6 additions & 0 deletions .github/workflows/python-tests.yml
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,12 @@ on:
- 'pmoves/tests/**'
- '.github/workflows/python-tests.yml'

permissions:
contents: read

permissions:
contents: read

jobs:
tests:
runs-on: ubuntu-latest
Expand Down
3 changes: 3 additions & 0 deletions .github/workflows/webhook-smoke.yml
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,9 @@ on:
required: false
default: "false"

permissions:
contents: read

jobs:
smoke:
runs-on: ubuntu-latest
Expand Down
Loading
Loading