feat(skill): comprehensive grind rewrite and cross-skill integration#896
feat(skill): comprehensive grind rewrite and cross-skill integration#896carlos-alm wants to merge 10 commits intomainfrom
Conversation
Wire up extracted helpers from Titan runs that existed but were never consumed, reducing boilerplate and improving error specificity. - Adopt named_child_text across 27 sites in 11 Rust extractors - Migrate cpp.rs from hand-rolled find_cpp_parent_class to find_enclosing_type_name - Add toSymbolRef helper in shared/normalize.ts, adopt at 15 mapping sites - Wire ParseError in parser.ts for structured PARSE_FAILED error codes - Wire BoundaryError in boundaries.ts to distinguish config/DB failures from clean results - Add --modules/--threshold flags to codegraph structure command - Wire batchQuery in CLI batch command, removing duplicated routing logic - Route detect-changes pending analysis through unified runAnalyses engine
- manifesto.ts: report 'warn' instead of 'pass' when boundary check throws - structure.ts: validate --threshold flag rejects non-numeric input - dependencies.ts: clarify intentional skip of toSymbolRef for callers
Forge extracts helpers but never completes the adoption loop — dead symbol count inflates with every run. Grind closes the gap by finding dead helpers from forge, classifying them (adopt/re-export/promote/ false-positive/remove), wiring them into consumers, and gating on a non-positive dead-symbol delta. Pipeline is now: recon → gauntlet → sync → forge → grind → close
- Track currentTarget, processedTargets, failedTargets in state for mid-run resume after interruption - Persist grind classifications to grind-targets.ndjson (append-only) so re-runs skip already-analyzed targets - Write titan-state.json after every target, not just at phase end - Add interrupted-mid-target recovery logic in edge cases - Use codegraph audit/context/fn-impact/where/query/ast before edits - Add codegraph diff-impact --staged before commits - Add codegraph build after edits to keep graph current - Add --target flag for retrying individual failures
Rewrite titan-grind with full resilience (state machine, .bak files, NDJSON persistence, snapshot management), codegraph integration (audit/context/fn-impact/diff-impact), diff review (DR1-DR3), drift detection, false positive tracking via issues.ndjson, and phase timestamps. Update titan-close: grind-targets.ndjson in artifact load list, adoption concern type in PR grouping, grind metrics in before/after comparison, GRIND row in Pipeline Timeline, Grind Results section in report template, grind block in close-summary.json. Update titan-reset: titan-grind-baseline snapshot deletion and grind-targets.ndjson in artifact listing.
Greptile SummaryThis PR rewrites
Confidence Score: 4/5Safe to merge after fixing the grind-targets.ndjson schema mismatch — all other source changes are clean refactors. One P1 finding: titan-close reads
|
| Filename | Overview |
|---|---|
| .claude/skills/titan-grind/SKILL.md | New 618-line skill implementing the full grind phase with state machine, NDJSON persistence, snapshot management, DR1-DR3 diff review, and codegraph integration — solid overall but writes a different NDJSON schema than titan-close expects to read. |
| .claude/skills/titan-close/SKILL.md | Adds grind artifact consumption (grind-targets.ndjson, grind metrics, GRIND pipeline row, Grind Results section), but reads status/deadBefore/deadAfter fields that titan-grind doesn't write — metrics will silently return zeros. |
| .claude/skills/titan-run/SKILL.md | Adds Step 4.5 grind loop with stall detection (V16/V17/V18/V19), updates description/args/phase sequence and pipeline summary — missing a pre-checks table row for --start-from grind. |
| .claude/skills/titan-reset/SKILL.md | Adds titan-grind-baseline snapshot deletion and grind-targets.ndjson to the artifact cleanup list — correct and complete. |
| src/shared/normalize.ts | Adds toSymbolRef helper to DRY up the repeated {name, kind, file, line} extraction pattern used across six analysis modules. |
| src/domain/analysis/dependencies.ts | Uses toSymbolRef for callees/candidates; intentionally keeps the manual mapping for callers because they carry the extra viaHierarchy field — comment correctly explains the deviation. |
| src/features/boundaries.ts | Changes invalid-config and query-failure paths from silent empty-return to throwing BoundaryError; both active callers (manifesto.ts, diff-impact.ts) already wrap in try-catch. |
| src/features/manifesto.ts | Adds try-catch around evaluateBoundaries to handle the newly-thrown BoundaryError, downgrading to a warn status — correct companion to the boundaries.ts change. |
| src/domain/graph/builder/stages/detect-changes.ts | Replaces conditional per-feature dynamic imports with a single runAnalyses call — logic is correct but the import is now unconditional even when neither CFG nor dataflow analysis is needed. |
| src/cli/commands/batch.ts | Moves multi-batch discrimination logic into presentation layer batchQuery; drops the MultiBatchItem interface and isMultiBatch guard — loses the targets[0] !== null null-safety check. |
| crates/codegraph-core/src/extractors/helpers.rs | Refactors find_enclosing_type_name, extract_constructor_name, and extract_call_name to use named_child_text helper consistently — clean consolidation. |
| src/domain/parser.ts | Required parser init failures now throw a structured ParseError with file and cause metadata instead of re-throwing the raw error — improves diagnostic context. |
Sequence Diagram
sequenceDiagram
participant Run as titan-run (orchestrator)
participant Forge as titan-forge
participant Grind as titan-grind
participant Gate as titan-gate
participant Close as titan-close
Run->>Forge: dispatch sub-agent (loop per phase)
Forge-->>Run: execution.completedPhases updated
Note over Run: Step 4.5 — new GRIND loop
loop for each unground forge phase
Run->>Grind: /titan-grind --phase N --yes
Grind->>Grind: classify dead helpers → grind-targets.ndjson
loop per adopt/remove target
Grind->>Grind: apply change, stage files
Grind->>Grind: DR1/DR2/DR3 diff review
Grind->>Gate: /titan-gate
Gate-->>Grind: pass / fail
Grind->>Grind: commit or rollback + snapshot restore
end
Grind->>Grind: dead-symbol delta gate
Grind-->>Run: grind.completedPhases updated
end
Run->>Close: dispatch sub-agent
Close->>Close: read grind-targets.ndjson + grind state
Close-->>Run: close-summary.json + PRs
Comments Outside Diff (1)
-
.claude/skills/titan-run/SKILL.md, line 141-143 (link)--start-from grindhas no pre-checks row in the validation tableThe table that maps each
startPhaseto the V-checks that must pass before it can run now includes a row forforge(V14, V15), but there is no row forgrind. When--start-from grindis used, the pre-checks for everything before grind should at minimum verify that the forge execution block exists intitan-state.jsonand thatexecution.completedPhasesis non-empty. Without a table row, the orchestrator has no guidance on which pre-checks to run when starting directly from grind.
Reviews (1): Last reviewed commit: "feat(skill): comprehensive grind rewrite..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
.claude/skills/titan-close/SKILL.md
Outdated
| - `.codegraph/titan/issues.ndjson` — issue tracker from all phases | ||
| - `.codegraph/titan/arch-snapshot.json` — pre-forge architectural snapshot (communities, structure, drift). Use for before/after comparison in the Metrics section. May not exist if capture failed. | ||
| - `.codegraph/titan/drift-report.json` — cumulative drift reports from all phases. May not exist if no drift was detected. | ||
| - `.codegraph/titan/grind-targets.ndjson` — grind phase adoption targets and outcomes. Each line: `{target, status, deadBefore, deadAfter, ...}`. May not exist if grind wasn't run. |
There was a problem hiding this comment.
titan-close expects entries with status: "done"|"failed"|"false_positive" and per-entry deadBefore/deadAfter fields, but titan-grind Step 2f writes entries with classification: "adopt|re-export|promote|..." and no dead-count fields. The grind metrics block in titan-close (count of status: "done", status: "failed", status: "false_positive") will always return 0, and the dead-symbol delta table will have no data.
titan-grind Step 2f writes:
{"target":"...","file":"...","phase":N,"classification":"adopt|remove|...","reason":"...","consumers":[...],"pattern":"...","timestamp":"..."}titan-close grind metrics reads:
- count of lines with `status: "done"`
- count of lines with `status: "failed"`
- `deadBefore`/`deadAfter` per target
The fix is to align the field names — either titan-grind should write a status field (mapped from classification) plus deadBefore/deadAfter, or titan-close should read classification and compute the counts from titan-state.json's grind.processedTargets/grind.failedTargets/grind.falsePositives lists instead.
There was a problem hiding this comment.
Fixed in 9c3752e. Updated titan-close to read classification from grind-targets.ndjson and pull metrics (processedTargets, failedTargets, falsePositives, deadSymbolDelta) from titan-state.json → grind block instead of the non-existent status/deadBefore/deadAfter fields. Also updated the artifact description on line 68 to document the actual NDJSON schema.
|
|
||
| const batchOpts = { | ||
| batchQuery(targets as Array<string | { command: string; target: string }>, opts.db, { | ||
| command, | ||
| depth: opts.depth ? parseInt(opts.depth as string, 10) : undefined, | ||
| file: opts.file, | ||
| kind: opts.kind, | ||
| noTests: ctx.resolveNoTests(opts), | ||
| }; | ||
|
|
||
| if (isMultiBatch(targets)) { | ||
| const data = multiBatchData(targets as MultiBatchItem[], opts.db, batchOpts); | ||
| console.log(JSON.stringify(data, null, 2)); | ||
| } else { | ||
| batch(command!, targets as string[], opts.db, batchOpts); | ||
| } | ||
| }); | ||
| }, | ||
| }; |
There was a problem hiding this comment.
The old isMultiBatch guard checked targets[0] !== null before 'command' in targets[0]. The new batchQuery checks typeof targets[0] === 'object' which is true for null, then immediately accesses (targets[0] as MultiBatchTarget).command. If a caller ever passes an array with a null first element (valid at runtime even if the union type excludes it), this will throw TypeError: Cannot read properties of null.
| const batchOpts = { | |
| batchQuery(targets as Array<string | { command: string; target: string }>, opts.db, { | |
| command, | |
| depth: opts.depth ? parseInt(opts.depth as string, 10) : undefined, | |
| file: opts.file, | |
| kind: opts.kind, | |
| noTests: ctx.resolveNoTests(opts), | |
| }; | |
| if (isMultiBatch(targets)) { | |
| const data = multiBatchData(targets as MultiBatchItem[], opts.db, batchOpts); | |
| console.log(JSON.stringify(data, null, 2)); | |
| } else { | |
| batch(command!, targets as string[], opts.db, batchOpts); | |
| } | |
| }); | |
| }, | |
| }; | |
| const isMulti = | |
| targets.length > 0 && | |
| targets[0] !== null && | |
| typeof targets[0] === 'object' && | |
| !!(targets[0] as MultiBatchTarget).command; |
There was a problem hiding this comment.
Fixed in e77e1f8. Added the targets[0] !== null guard back before the typeof check, since typeof null === 'object' is true in JS and would cause a TypeError when accessing .command.
| rootDir, | ||
| analysisOpts, | ||
| ); | ||
| if (needsCfg) { | ||
| const { buildCFGData } = await import('../../../../features/cfg.js'); | ||
| await buildCFGData(db, analysisSymbols, rootDir, engineOpts); | ||
| } | ||
| if (needsDataflow) { | ||
| const { buildDataflowEdges } = await import('../../../../features/dataflow.js'); | ||
| await buildDataflowEdges(db, analysisSymbols, rootDir, engineOpts); | ||
| } | ||
| const { runAnalyses } = await import('../../../../ast-analysis/engine.js'); | ||
| await runAnalyses( | ||
| db, | ||
| analysisSymbols, | ||
| rootDir, | ||
| { ast: false, complexity: false, cfg: needsCfg, dataflow: needsDataflow }, | ||
| engineOpts, | ||
| ); | ||
| return true; | ||
| } | ||
|
|
There was a problem hiding this comment.
The old code only dynamically imported cfg.js / dataflow.js when the respective flag was set. The new code always imports ast-analysis/engine.js, even when both needsCfg and needsDataflow are false (the common incremental-build path). This adds a module-load round-trip on every incremental rebuild that requires no supplemental analysis.
Consider guarding the import:
if (needsCfg || needsDataflow) {
const { runAnalyses } = await import('../../../../ast-analysis/engine.js');
await runAnalyses(db, analysisSymbols, rootDir,
{ ast: false, complexity: false, cfg: needsCfg, dataflow: needsDataflow },
engineOpts,
);
}There was a problem hiding this comment.
This is actually already guarded. Line 268 has if (!needsCfg && !needsDataflow) return false; which early-returns before the import on line 280 ever executes. The import is only reached when at least one of needsCfg/needsDataflow is true, so there's no unnecessary module-load on the incremental-build path.
…chema (#896) titan-close was reading status/deadBefore/deadAfter fields that titan-grind never writes. Updated to read classification field from grind-targets.ndjson and pull metrics from titan-state.json grind block.
typeof null === 'object' is true in JS, so accessing .command on null would throw TypeError. Added explicit null check before the typeof.
Without a validation table row for grind, the orchestrator had no guidance on which pre-checks to run when starting directly from grind.
|
Re: P2 — Fixed in 0718ddf. Added a |
Codegraph Impact Analysis1 functions changed → 1 callers affected across 1 files
|
1 participant
Summary
adoptionconcern type in PR grouping, grind metrics in before/after comparison, GRIND row in Pipeline Timeline, Grind Results section in report templateContext
Follow-up to #895 (dead helper adoption). The initial grind skill was minimal — this rewrite addresses 25 gaps found by comparing against all other titan skills (forge, gate, close, reset, sync, gauntlet, recon).
Test plan