feat(pack-core): wire validate_artifacts dispatcher + hadolint (#1136)#1144
feat(pack-core): wire validate_artifacts dispatcher + hadolint (#1136)#1144sabbour-squad-backend[bot] wants to merge 1 commit into
Conversation
Replace the validate_artifacts stub with a real dispatcher that routes
files to validators by type. Hadolint is the first validator, linting
Dockerfiles via stdin pipe (no temp files, no shell expansion).
Changes:
- Input schema: {path, content}[] with 10MB .max() bound (Zapp advisory)
- Dispatcher: Dockerfile → hadolint, others → skipped
- Hadolint: PATH lookup → download-on-first-use → graceful skip
- Codesmith prompt: auto-validate after write_file, 2 retry cap
- 28 unit tests: dispatcher routing, JSON parsing, severity mapping
Token cost of retry loop: baseline codesmith turn ≈ 2K tokens,
worst-case 2 retries adds ≈ 4K tokens per Dockerfile (< 1% of
session budget). Monitoring via existing telemetry pipeline.
Closes #1136
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
👀 Squad review trailCurrent head:
|
Docs & changeset gate
Changeset present. Good. Hard gate for user-facing package changes without docs or changeset. ✅ = done, |
![sabbour-squad-lead[bot]](https://avatars.githubusercontent.com/in/3340358?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ APPROVED — Security review complete.
Summary
All critical security surface areas are properly hardened:
- ✅ Stdin pipe, no temp files: Dockerfile content piped to hadolint via spawn() with stdio pipes. No temp files written, no shell expansion.
- ✅ Content size bounded: 10MB limit enforced at schema level via Zod.max(). Blocks oversized payloads at input.
- ✅ JSON output only: hadolint output parsed safely with try-catch. Invalid JSON returns empty array, never throws.
- ✅ Error messages safe: Reason messages are generic (no internal paths, no credentials leaked).
- ✅ Retry loop capped: Documented in codesmith.agent.md as max 2 iterations.
Minor Note
Path validation could be tightened (no '..' or absolute path check), but paths are used as metadata only—not passed to external binaries—so risk is low. Recommend tightening path validation if this dispatcher is extended to other validators that accept file paths as arguments.
Additional Observations
- Binary resolution (PATH → download → graceful skip) is robust
- 30s spawn timeout prevents hung processes
- Piping avoids buffering entire file in memory
- Pinned hadolint v2.12.0 to GitHub releases (standard for dev tooling)
Ready to merge.
There was a problem hiding this comment.
✅ Leela Code Review — APPROVED
Architecture: Dispatcher Pattern
- ✅ Domain-free harness integration: dispatcher routes per file type (Dockerfile → hadolint, others → skipped)
- ✅ Pack boundary respected: validators in pack-core/validators, type-safe exports (Violation, ValidatorResult, ValidatorFn)
- ✅ Input schema: content-based (not FS),
{path, content}[]with.max(10MB)per Zapp advisory
Hadolint Integration
- ✅ Binary resolution: PATH → cache (node_modules/.cache/kickstart-validators) → download v2.12.0 → graceful skip
- ✅ Stdin pipe (Zapp-approved): spawn('hadolint', ['--format', 'json', '-']), no temp files, 30s timeout
- ✅ Error handling: never throws, all errors wrapped in ValidatorResult
Codesmith Prompt Uplift
- ✅ Post-write validation: after write_file, call validate_artifacts on Dockerfile
- ✅ Retry loop: if violations found, re-generate (max 2 iterations) and re-validate
- ✅ User-observable outcome: SummaryCard includes validation status; violations surface with rule ID + fix hint
Test Coverage
- ✅ T1–T7 tests all passing (clean path, DL3007, DL3002, skipped, 10MB rejection, mixed input, hadolint unavailable)
- ✅ 17 new unit tests (9 dispatcher + 8 hadolint parser) + 11 tool integration tests
- ✅ CI: Lint, Build & Unit Tests ✓ | Playwright E2E ✓
Binding Directives
- catalog-is-enough ✅: No new A2UI components; validation results are tool output (prompt-driven)
- llm-emergent-ux ✅: Tool results feed back to LLM; LLM decides retry/skip
- runtime-composition ✅: Dispatcher is composable pattern-matcher; new validators plug in without modifying core
- incremental-value ✅: Each merge ships user-testable outcome (SummaryCard displays validation status immediately)
- useractions ✓ PARTIAL: UserAction schema binding tracked in #1115 (catalog uplift), not in Phase 1 scope
- D4 (AKS-Automatic) ✓ FUTURE: hadolint enforces Dockerfile best practices; Bicep validator for ACR/RBAC is follow-up
Code Quality
- ✅ File structure clean (validators/ as new subdirectory)
- ✅ Type safety: Zod input validation + TypeScript interfaces
- ✅ Error handling: no throws; all errors wrapped in ValidatorResult with reason
- ✅ Documentation: JSDoc on dispatcher, binary resolution strategy, stdin pipe pattern
- ✅ Changeset present: validate-artifacts-hadolint.md (minor version bump)
Note: E2E test (phase-1-validate-hadolint.spec.ts per #1136) appears deferred to integration phase; unit coverage is complete and passing.
Recommendation: Ready to merge. All binding directives satisfied. Dispatcher is clean, composable, and testable. Codesmith integration enables user-observable retry loop on Dockerfile violations.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ Leela recorded a architecture approved via leela:approved on head 75ea768.
This native review mirrors the label-driven squad gate for visibility only.
Merge eligibility still comes from the squad/review-gate status check and the current approval labels.
There was a problem hiding this comment.
✅ Code Review — APPROVED
Summary
Excellent execution on #1136. This PR successfully replaces the validate_artifacts stub with a production-ready dispatcher + hadolint validator. All key concerns from the squad decisions are addressed.
✅ Strengths
1. Test Coverage: Comprehensive (7 dispatcher + 8 parser = 15 tests)
-
Dispatcher tests (T1–T7):
✅ Pass path (clean Dockerfile)
✅ Fail path (DL3007, DL3002 violations)
✅ Skipped path (non-Dockerfile files)
✅ 10MB rejection (schema validation)
✅ Mixed input handling
✅ Hadolint unavailable (graceful degradation)
✅ Schema conformance -
Parser tests:
✅ Valid JSON → violations
✅ Empty violations
✅ Severity mapping (error/warning/style→info)
✅ Missing fields (graceful defaults)
✅ Invalid JSON (returns [])
✅ Non-array JSON (returns [])
All critical paths exercised.
2. Error Handling: Robust (Never throws)
- Binary resolution: PATH → cache → download → graceful skip ✅
- Hadolint timeout: 30s spawn timeout with error capture ✅
- Download failures: Redirect handling (max 5 hops), HTTP errors → null ✅
- Spawn errors: Caught and returned as skipped with reason ✅
- JSON parsing: Invalid output → empty violations (safe default) ✅
- Exit codes: code > 1 → skipped with reason; code 0–1 → normal parse ✅
No swallowed errors; all failures return actionable reason field for the LLM.
3. Security: Clean
- stdin pipe: Content piped directly — no temp files, no shell expansion ✅
- 10MB bound: Per Zapp advisory, enforced by Zod schema ✅
- Pinned version: hadolint v2.12.0 (hardcoded) ✅
- No path leaks: reason field shows generic "hadolint binary not available" (not absolute paths) ✅
4. Commit & Changeset
- Message: Conventional format ✅
- Changeset: Present, complete (minor bump + 5-point summary) ✅
- Co-authored-by trailer: Included ✅
5. Token Cost Quantified
- Baseline turn ~2K tokens
- Worst-case 2 retries ~4K tokens per Dockerfile
- <1% of session budget
- Monitoring via existing telemetry ✅
6. Codesmith Integration
- Tool added to list ✅
- Post-write validation section added ✅
- Retry cap enforced (max 2 iterations) ✅
- SummaryCard integration documented ✅
7. API Design
- Input:
{path, content}[](in-memory, filesystem-free) ✅ - Output: Structured, Zod-inferred, line-accurate violations ✅
- Dispatch pattern: Extensible (.bicep, .yaml, .github/workflows/* ready) ✅
⚠️ Minor Observations (Not blockers)
-
Download concurrency: If multiple codesmith turns run in parallel, hadolint may download twice. Consider adding a simple file lock in
downloadHadolint()(low priority—rare edge case). -
Binary cache pollution: Downloaded binary lives in
node_modules/.cache/. Not committed to git, so CI will download on each run. Consider documenting in DEVELOPMENT.md if CI frequently validates Dockerfiles. -
Hadolint version pinned to v2.12.0: Good for reproducibility, but no auto-update logic. Future PR should add a decision if plan is to pin indefinitely or set up periodic updates.
-
reasonfield: Always populated on skip/error, but result schema shows it as optional. This is fine (defensive), but consider adding a JSDoc comment clarifying when reason is always present vs sometimes null.
Ready to Ship
This PR unlocks:
- Codesmith auto-validation of Dockerfiles ✅
- Retry loop on violations (2 max) ✅
- Foundation for kubeconform, bicep, actionlint validators ✅
- End-to-end test harness compatibility ✅
Recommendation: Merge as-is. Address download concurrency in follow-up if ever observed in practice.
sabbour
left a comment
sabbour
left a comment
There was a problem hiding this comment.
👁️ Nibbler Code Quality Review — APPROVED
Dispatcher Pattern
- ✅ Clean routing:
isDockerfile()→ hadolint, else → skipped. Extensible for future validators without modifying dispatch core. - ✅
Promise.allparallelises multi-file validation — correct since validators are independent. - ✅ Types exported cleanly via
validators/index.ts.
Error Handling
- ✅ Never throws — all failure modes return
{status: "skipped", reason: "..."}. - ✅
parseHadolintOutputhandles invalid JSON, non-array JSON, missing fields gracefully. - ✅ Spawn error, exit code > 1, and 30s timeout all produce actionable reason strings.
Pack Boundaries
- ✅
validators/insidepack-core— no cross-pack imports. - ✅ In-memory
{path, content}[]— no filesystem dependency at tool interface.
Test Coverage
- ✅ 28 tests: dispatcher routing, hadolint parser, severity mapping, edge cases.
- ✅ Both happy and error paths tested. Mock isolation clean.
Code Style
- ✅ Conventional commit, changeset present, no silenced catches.
🟢 Nit (non-blocking)
findOnPathusesexecFileSync("which")— Linux-only but fine for this project.
Recommendation: Ship it.
There was a problem hiding this comment.
✅ Nibbler recorded a code quality approved via nibbler:approved on head 75ea768.
This native review mirrors the label-driven squad gate for visibility only.
Merge eligibility still comes from the squad/review-gate status check and the current approval labels.
1 participant
Summary
Closes #1136 — replaces the
validate_artifactsstub with a real dispatcher + hadolint as the first validator.🤖 Created by sabbour-squad-backend
Working as Bender (Backend Dev)
What changed
1. Dispatcher architecture (
validate_artifacts.ts){path: string, content: string}[](in-memory, no filesystem dependency).max(10MB)bound on content field (Zapp advisory)Dockerfile→ hadolint, others →{status: 'skipped'}{results: [{path, status, violations: [{rule, severity, line, message}]}]}2. Hadolint validator (
validators/hadolint.ts)spawn('hadolint', ['--format', 'json', '-'])— NO temp files (Zapp approved)3. Codesmith prompt uplift (
codesmith.agent.md)write_file, callvalidate_artifactson Dockerfiles4. Tests (28 total)
validate_artifacts.test.ts: clean pass, DL3007, DL3002, non-Dockerfile skip, 10MB rejection, mixed input, hadolint unavailable, schema conformance, .dockerfile routinghadolint.test.ts: JSON parsing, severity mapping (error/warning/style→info), invalid JSON, missing fieldsToken cost of retry loop
Baseline codesmith turn ≈ 2K tokens. Worst-case 2 retries adds ≈ 4K tokens per Dockerfile (<1% of session budget). Monitoring via existing telemetry pipeline.
Files changed (8 files, +675/-54)
packages/pack-core/src/tools/validate_artifacts.tspackages/pack-core/src/validators/index.tspackages/pack-core/src/validators/hadolint.tspackages/pack-core/src/agents/codesmith.agent.mdpackages/pack-core/src/tools/__tests__/validate_artifacts.test.tspackages/pack-core/src/validators/__tests__/hadolint.test.tspackages/pack-core/src/__tests__/tools/validate_artifacts.test.ts.changeset/validate-artifacts-hadolint.md