mirror of
https://github.com/garrytan/gstack.git
synced 2026-05-21 20:28:24 +08:00
fix: atomic review log helpers + platform-agnostic templates (v0.8.5) (#209)
* fix: add gstack-review-log and gstack-review-read atomic helpers Branch names with `/` break review log filepaths when Claude Code runs multi-line bash blocks as separate shell invocations. These two scripts encapsulate the full operation in a single command. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: replace multi-line eval+mkdir+echo blocks with atomic helpers - Review log writes now use gstack-review-log (single command) - Review dashboard reads now use gstack-review-read (single command) - Remaining source+mkdir blocks use && chaining for variable persistence - Regenerated all SKILL.md files Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: remove Rails-isms — platform-agnostic templates and checklist - review/checklist.md: multi-framework examples (Rails/Node/Python/Django) - plan-ceo-review: framework-agnostic grep + generic error table - plan-eng-review: "corresponding test" not "JS or Rails test" - CLAUDE.md: Platform-agnostic design principle + Testing section Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * test: update tests for gstack-review-log/read helpers - codex review log test: check for gstack-review-log instead of reviews.jsonl - dashboard resolver tests: check for gstack-review instead of reviews.jsonl Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * chore: bump version and changelog (v0.8.5) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -294,9 +294,7 @@ source <(~/.claude/skills/gstack/bin/gstack-diff-scope <base> 2>/dev/null)
|
||||
6. **Log the result** for the Review Readiness Dashboard:
|
||||
|
||||
```bash
|
||||
source <(~/.claude/skills/gstack/bin/gstack-slug 2>/dev/null)
|
||||
mkdir -p ~/.gstack/projects/$SLUG
|
||||
echo '{"skill":"design-review-lite","timestamp":"TIMESTAMP","status":"STATUS","findings":N,"auto_fixed":M,"commit":"COMMIT"}' >> ~/.gstack/projects/$SLUG/$BRANCH-reviews.jsonl
|
||||
~/.claude/skills/gstack/bin/gstack-review-log '{"skill":"design-review-lite","timestamp":"TIMESTAMP","status":"STATUS","findings":N,"auto_fixed":M,"commit":"COMMIT"}'
|
||||
```
|
||||
|
||||
Substitute: TIMESTAMP = ISO 8601 datetime, STATUS = "clean" if 0 findings or "issues_found", N = total findings, M = auto-fixed count, COMMIT = output of `git rev-parse --short HEAD`.
|
||||
@@ -453,10 +451,7 @@ Present the full output verbatim under a `CODEX SAYS (adversarial challenge):` h
|
||||
|
||||
**Only if a code review ran (user chose A or C):** Persist the Codex review result to the review log:
|
||||
```bash
|
||||
source <(~/.claude/skills/gstack/bin/gstack-slug 2>/dev/null)
|
||||
BRANCH_SLUG=$(git rev-parse --abbrev-ref HEAD 2>/dev/null | tr '/' '-')
|
||||
mkdir -p ~/.gstack/projects/"$SLUG"
|
||||
echo '{"skill":"codex-review","timestamp":"'"$(date -u +%Y-%m-%dT%H:%M:%SZ)"'","status":"STATUS","gate":"GATE"}' >> ~/.gstack/projects/"$SLUG"/"$BRANCH_SLUG"-reviews.jsonl
|
||||
~/.claude/skills/gstack/bin/gstack-review-log '{"skill":"codex-review","timestamp":"'"$(date -u +%Y-%m-%dT%H:%M:%SZ)"'","status":"STATUS","gate":"GATE"}'
|
||||
```
|
||||
|
||||
Substitute: STATUS ("clean" if PASS, "issues_found" if FAIL), GATE ("pass" or "fail").
|
||||
|
||||
@@ -267,10 +267,7 @@ Present the full output verbatim under a `CODEX SAYS (adversarial challenge):` h
|
||||
|
||||
**Only if a code review ran (user chose A or C):** Persist the Codex review result to the review log:
|
||||
```bash
|
||||
source <(~/.claude/skills/gstack/bin/gstack-slug 2>/dev/null)
|
||||
BRANCH_SLUG=$(git rev-parse --abbrev-ref HEAD 2>/dev/null | tr '/' '-')
|
||||
mkdir -p ~/.gstack/projects/"$SLUG"
|
||||
echo '{"skill":"codex-review","timestamp":"'"$(date -u +%Y-%m-%dT%H:%M:%SZ)"'","status":"STATUS","gate":"GATE"}' >> ~/.gstack/projects/"$SLUG"/"$BRANCH_SLUG"-reviews.jsonl
|
||||
~/.claude/skills/gstack/bin/gstack-review-log '{"skill":"codex-review","timestamp":"'"$(date -u +%Y-%m-%dT%H:%M:%SZ)"'","status":"STATUS","gate":"GATE"}'
|
||||
```
|
||||
|
||||
Substitute: STATUS ("clean" if PASS, "issues_found" if FAIL), GATE ("pass" or "fail").
|
||||
|
||||
@@ -35,16 +35,16 @@ Be terse. For each issue: one line describing the problem, one line with the fix
|
||||
### Pass 1 — CRITICAL
|
||||
|
||||
#### SQL & Data Safety
|
||||
- String interpolation in SQL (even if values are `.to_i`/`.to_f` — use `sanitize_sql_array` or Arel)
|
||||
- String interpolation in SQL (even if values are `.to_i`/`.to_f` — use parameterized queries (Rails: sanitize_sql_array/Arel; Node: prepared statements; Python: parameterized queries))
|
||||
- TOCTOU races: check-then-set patterns that should be atomic `WHERE` + `update_all`
|
||||
- `update_column`/`update_columns` bypassing validations on fields that have or should have constraints
|
||||
- N+1 queries: `.includes()` missing for associations used in loops/views (especially avatar, attachments)
|
||||
- Bypassing model validations for direct DB writes (Rails: update_column; Django: QuerySet.update(); Prisma: raw queries)
|
||||
- N+1 queries: Missing eager loading (Rails: .includes(); SQLAlchemy: joinedload(); Prisma: include) for associations used in loops/views
|
||||
|
||||
#### Race Conditions & Concurrency
|
||||
- Read-check-write without uniqueness constraint or `rescue RecordNotUnique; retry` (e.g., `where(hash:).first` then `save!` without handling concurrent insert)
|
||||
- `find_or_create_by` on columns without unique DB index — concurrent calls can create duplicates
|
||||
- Read-check-write without uniqueness constraint or catch duplicate key error and retry (e.g., `where(hash:).first` then `save!` without handling concurrent insert)
|
||||
- find-or-create without unique DB index — concurrent calls can create duplicates
|
||||
- Status transitions that don't use atomic `WHERE old_status = ? UPDATE SET new_status` — concurrent updates can skip or double-apply transitions
|
||||
- `html_safe` on user-controlled data (XSS) — check any `.html_safe`, `raw()`, or string interpolation into `html_safe` output
|
||||
- Unsafe HTML rendering (Rails: .html_safe/raw(); React: dangerouslySetInnerHTML; Vue: v-html; Django: |safe/mark_safe) on user-controlled data (XSS)
|
||||
|
||||
#### LLM Output Trust Boundary
|
||||
- LLM-generated values (emails, URLs, names) written to DB or passed to mailers without format validation. Add lightweight guards (`EMAIL_REGEXP`, `URI.parse`, `.strip`) before persisting.
|
||||
@@ -141,7 +141,7 @@ the agent auto-fixes a finding or asks the user.
|
||||
```
|
||||
AUTO-FIX (agent fixes without asking): ASK (needs human judgment):
|
||||
├─ Dead code / unused variables ├─ Security (auth, XSS, injection)
|
||||
├─ N+1 queries (missing .includes()) ├─ Race conditions
|
||||
├─ N+1 queries (missing eager loading) ├─ Race conditions
|
||||
├─ Stale comments contradicting code ├─ Design decisions
|
||||
├─ Magic numbers → named constants ├─ Large fixes (>20 lines)
|
||||
├─ Missing LLM output validation ├─ Enum completeness
|
||||
|
||||
Reference in New Issue
Block a user