Conversation
…err.E() Replace all 8 instances of fmt.Errorf across parser.go, html.go, and video.go with coreerr.E() from forge.lthn.ai/core/go-log for consistent error handling. Add dependency on go-log v0.0.4. Co-Authored-By: Virgil <virgil@lethean.io>
Fix two coreerr.E() calls missing the required third (error) argument, add SPDX-Licence-Identifier headers to all source files, and add tests for PruneSessions, IsExpired, and FetchSession (coverage 88.1% → 92.1%). Co-Authored-By: Virgil <virgil@lethean.io>
…LAUDE.md — update any outdate...' (#1) from agent/dx-audit-and-fix--1--review-claude-md into main Reviewed-on: https://forge.lthn.ai/core/go-session/pulls/1
…AUDE.md - Fix coreerr.E() calls missing third arg in FetchSession and RenderMP4 - Add SPDX-Licence-Identifier headers to parser.go, html.go, video.go, search.go - Add tests for PruneSessions, IsExpired, and FetchSession (coverage 87.7% → 92.1%) - Document coreerr.E() error handling convention in CLAUDE.md - No fmt.Errorf or os.ReadFile/os.WriteFile violations found in source files Co-Authored-By: Virgil <virgil@lethean.io>
…LAUDE.md — update any outdate...' (#2) from agent/dx-audit-and-fix--1--review-claude-md into main
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughAdds many new documentation and knowledge-base files, a task protocol and agent guidance, introduces Session/Event documentation and API surface descriptions, updates go.mod to add a core error library, and replaces error construction in several Go files with the new coreerr.E() wrapper. Changes
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📝 Coding Plan
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.11.3)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (4)
RECENT.md (1)
3-24: Consider adding a language identifier to the fenced code block.The commit log block lacks a language specifier. Using
textorplaintextwould satisfy MD040 and maintain consistency with other documentation files.📝 Suggested fix
-``` +```text a07e41a chore: add .core/ and .idea/ to .gitignore ... ```🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@RECENT.md` around lines 3 - 24, The fenced commit log block (the triple-backtick block containing commit hashes like a07e41a, 50d1c3f, etc.) is missing a language identifier; update the opening fence from ``` to ```text (or ```plaintext) to satisfy MD040 and match other docs, ensuring the code block is still preserved and renders as plain text.kb/Rendering.md (1)
95-102: Consider adding a language identifier to the fenced code block.The tape configuration block lacks a language specifier, which may affect syntax highlighting in some renderers. A simple identifier like
textoriniwould satisfy MD040.📝 Suggested fix
-``` +```text FontSize 16 Width 1400 Height 800 TypingSpeed 30ms Theme "Catppuccin Mocha" Shell bash ```🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@kb/Rendering.md` around lines 95 - 102, The fenced code block containing the lines "FontSize 16", "Width 1400", "Height 800", "TypingSpeed 30ms", "Theme \"Catppuccin Mocha\"", and "Shell bash" should include a language identifier for proper syntax highlighting; update the opening fence from ``` to something like ```text (or ```ini) so the block becomes a fenced code block with a language specifier.specs/TASK_PROTOCOL.md (1)
28-28: Add fence language identifiers to satisfy markdownlint.These fenced blocks are untyped; adding a language (for example
textormarkdown) keeps docs lint-clean.Also applies to: 214-214, 231-231, 252-252, 344-344, 356-356, 386-386, 521-521
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@specs/TASK_PROTOCOL.md` at line 28, Several fenced code blocks in TASK_PROTOCOL.md are untyped (plain ```), which triggers markdownlint; update each triple-backtick fence (the anonymous fenced blocks present in the file, including the occurrences noted) to include a language identifier such as text or markdown (e.g., change ``` to ```text or ```markdown) so the blocks are explicitly typed and lint-clean; search for anonymous fences in TASK_PROTOCOL.md and replace them accordingly.CONTEXT.md (1)
5-5: Address markdownlint warnings in headings and code fences.Heading level jump and untyped fenced blocks may fail docs linting.
Suggested fix
-### 1. go-session [convention] (score: 0.636) +## 1. go-session [convention] (score: 0.636)-``` +```text FontSize 16 Width 1400 Height 800 TypingSpeed 30ms Theme "Catppuccin Mocha" Shell bash</details> Also applies to: 127-127 <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@CONTEXT.mdat line 5, Fix the markdownlint issues by adjusting the heading
levels for the "go-session" heading to avoid level jumps (ensure it follows the
surrounding headings' hierarchy) and update the untyped fenced code blocks by
adding a language identifier (e.g., replacewithtext) for the block that
contains the FontSize/Width/Height/TypingSpeed/Theme/Shell snippet so the docs
linter passes; locate and update the "go-session" heading and the adjacent
fenced block in CONTEXT.md accordingly.</details> </blockquote></details> </blockquote></details> <details> <summary>🤖 Prompt for all review comments with AI agents</summary>Verify each finding against the current code and only fix it if needed.
Inline comments:
In@CONTEXT.md:
- Around line 9-11: Replace the machine-local absolute paths with repo-relative
paths: update the three lines referencing
/Users/snider/Code/go-session/docs/architecture.md,
/Users/snider/Code/go-session/docs/development.md, and
/Users/snider/Code/go-session/docs/history.mdto use repo-relative paths such
asdocs/architecture.md,docs/development.md, anddocs/history.mdso links
resolve for all contributors; ensure any other absolute-path occurrences in
CONTEXT.md are converted similarly and verify links open correctly in the
repository.In
@kb/Home.md:
- Line 5: Update the documentation sentence in Home.md that currently claims
"Zero external dependencies (stdlib only)" to accurately reflect the new
external dependency introduced by the PR; mention the added package
forge.lthn.ai/core/go-log v0.0.4 (or more generally "external logging
dependency") in the description for the project and replace the absolute claim
with a correct statement such as "minimal external dependencies (includes
forge.lthn.ai/core/go-log v0.0.4 for logging)"; edit the phrase in kb/Home.md to
reference that package name so readers see the dependency and its purpose.In
@specs/AGENT_CONTEXT.md:
- Line 193: Update the internal protocol link string 'doc/TASK_PROTOCOL.md' to
the correct location 'specs/TASK_PROTOCOL.md' in the AGENT_CONTEXT.md content so
the verification workflow link points to the actual protocol file; search for
the literal 'doc/TASK_PROTOCOL.md' and replace it with 'specs/TASK_PROTOCOL.md'
to restore discoverability.In
@specs/TASK_PROTOCOL.md:
- Line 570: Update the inconsistent TODO path: change the string "Update TODO.md
with the new task" to use the same prefixed path used elsewhere, i.e., "Update
tasks/TODO.md with the new task" so the protocol consistently references
tasks/TODO.md.- Line 30: Update the documentation text in TASK_PROTOCOL.md that claims the
file is located under "doc/" to reflect the correct location "specs/"; locate
the inaccurate directory reference in TASK_PROTOCOL.md (look for the line or
sentence that mentions "doc/") and replace it with "specs/" and optionally add a
brief note clarifying that the file resides in the specs directory to avoid
future confusion.In
@TODO.md:
- Line 1: The TODO entry saying "Replace ALL fmt.Errorf and errors.New in
production code with coreerr.E() from go-log" is stale because the PR already
made those replacements; update the TODO by removing that line or marking it
completed (e.g., strike-through or a "Done" note) so it doesn't reopen
work—locate the TODO text fragment "Replace ALL fmt.Errorf and errors.New in
production code with coreerr.E()" in TODO.md and either delete it or annotate it
as completed and run the test suite to confirm nothing else is required.
Nitpick comments:
In@CONTEXT.md:
- Line 5: Fix the markdownlint issues by adjusting the heading levels for the
"go-session" heading to avoid level jumps (ensure it follows the surrounding
headings' hierarchy) and update the untyped fenced code blocks by adding a
language identifier (e.g., replacewithtext) for the block that contains
the FontSize/Width/Height/TypingSpeed/Theme/Shell snippet so the docs linter
passes; locate and update the "go-session" heading and the adjacent fenced block
in CONTEXT.md accordingly.In
@kb/Rendering.md:
- Around line 95-102: The fenced code block containing the lines "FontSize 16",
"Width 1400", "Height 800", "TypingSpeed 30ms", "Theme "Catppuccin Mocha"",
and "Shell bash" should include a language identifier for proper syntax
highlighting; update the opening fence fromto something liketext (orIn `@RECENT.md`: - Around line 3-24: The fenced commit log block (the triple-backtick block containing commit hashes like a07e41a, 50d1c3f, etc.) is missing a language identifier; update the opening fence from ``` to ```text (or ```plaintext) to satisfy MD040 and match other docs, ensuring the code block is still preserved and renders as plain text. In `@specs/TASK_PROTOCOL.md`: - Line 28: Several fenced code blocks in TASK_PROTOCOL.md are untyped (plain ```), which triggers markdownlint; update each triple-backtick fence (the anonymous fenced blocks present in the file, including the occurrences noted) to include a language identifier such as text or markdown (e.g., change ``` to ```text or ```markdown) so the blocks are explicitly typed and lint-clean; search for anonymous fences in TASK_PROTOCOL.md and replace them accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID:
ea54c071-a083-4795-b382-e783e01f808a⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum📒 Files selected for processing (14)
CONTEXT.mdGEMINI.mdPROMPT.mdRECENT.mdTODO.mdgo.modhtml.gokb/Home.mdkb/Rendering.mdkb/Session-Format.mdparser.gospecs/AGENT_CONTEXT.mdspecs/TASK_PROTOCOL.mdvideo.go
| - `/Users/snider/Code/go-session/docs/architecture.md` — JSONL format, parsing pipeline, event types, analytics, HTML rendering, XSS protection | ||
| - `/Users/snider/Code/go-session/docs/development.md` — prerequisites, build/test commands, test patterns, coding standards | ||
| - `/Users/snider/Code/go-session/docs/history.md` — completed phases, known limitations, future considerations |
There was a problem hiding this comment.
Replace machine-local absolute paths with repo-relative paths.
These links are tied to one workstation and will not resolve for other contributors.
Suggested fix
-- `/Users/snider/Code/go-session/docs/architecture.md` — JSONL format, parsing pipeline, event types, analytics, HTML rendering, XSS protection
-- `/Users/snider/Code/go-session/docs/development.md` — prerequisites, build/test commands, test patterns, coding standards
-- `/Users/snider/Code/go-session/docs/history.md` — completed phases, known limitations, future considerations
+- `docs/architecture.md` — JSONL format, parsing pipeline, event types, analytics, HTML rendering, XSS protection
+- `docs/development.md` — prerequisites, build/test commands, test patterns, coding standards
+- `docs/history.md` — completed phases, known limitations, future considerations📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - `/Users/snider/Code/go-session/docs/architecture.md` — JSONL format, parsing pipeline, event types, analytics, HTML rendering, XSS protection | |
| - `/Users/snider/Code/go-session/docs/development.md` — prerequisites, build/test commands, test patterns, coding standards | |
| - `/Users/snider/Code/go-session/docs/history.md` — completed phases, known limitations, future considerations | |
| - `docs/architecture.md` — JSONL format, parsing pipeline, event types, analytics, HTML rendering, XSS protection | |
| - `docs/development.md` — prerequisites, build/test commands, test patterns, coding standards | |
| - `docs/history.md` — completed phases, known limitations, future considerations |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@CONTEXT.md` around lines 9 - 11, Replace the machine-local absolute paths
with repo-relative paths: update the three lines referencing
`/Users/snider/Code/go-session/docs/architecture.md`,
`/Users/snider/Code/go-session/docs/development.md`, and
`/Users/snider/Code/go-session/docs/history.md` to use repo-relative paths such
as `docs/architecture.md`, `docs/development.md`, and `docs/history.md` so links
resolve for all contributors; ensure any other absolute-path occurrences in
CONTEXT.md are converted similarly and verify links open correctly in the
repository.
|
|
||
| `forge.lthn.ai/core/go-session` -- Claude Code session parser and visualiser. | ||
|
|
||
| Reads JSONL transcript files produced by Claude Code, extracts structured events, and renders them as interactive HTML timelines or MP4 videos. Zero external dependencies (stdlib only). |
There was a problem hiding this comment.
Documentation is now inaccurate — package has external dependency.
The claim "Zero external dependencies (stdlib only)" conflicts with the PR's addition of forge.lthn.ai/core/go-log v0.0.4 for error handling. Consider updating to reflect the actual dependency.
📝 Suggested fix
-Reads JSONL transcript files produced by Claude Code, extracts structured events, and renders them as interactive HTML timelines or MP4 videos. Zero external dependencies (stdlib only).
+Reads JSONL transcript files produced by Claude Code, extracts structured events, and renders them as interactive HTML timelines or MP4 videos. Minimal external dependencies.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Reads JSONL transcript files produced by Claude Code, extracts structured events, and renders them as interactive HTML timelines or MP4 videos. Zero external dependencies (stdlib only). | |
| Reads JSONL transcript files produced by Claude Code, extracts structured events, and renders them as interactive HTML timelines or MP4 videos. Minimal external dependencies. |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@kb/Home.md` at line 5, Update the documentation sentence in Home.md that
currently claims "Zero external dependencies (stdlib only)" to accurately
reflect the new external dependency introduced by the PR; mention the added
package forge.lthn.ai/core/go-log v0.0.4 (or more generally "external logging
dependency") in the description for the project and replace the absolute claim
with a correct statement such as "minimal external dependencies (includes
forge.lthn.ai/core/go-log v0.0.4 for logging)"; edit the phrase in kb/Home.md to
reference that package name so readers see the dependency and its purpose.
| ## Getting Help | ||
|
|
||
| - Check `tasks/` for active task specs | ||
| - Check `doc/TASK_PROTOCOL.md` for the verification workflow |
There was a problem hiding this comment.
Fix internal protocol path reference for discoverability.
The path currently points to doc/TASK_PROTOCOL.md, but the protocol file here is in specs/.
Suggested fix
-- Check `doc/TASK_PROTOCOL.md` for the verification workflow
+- Check `specs/TASK_PROTOCOL.md` for the verification workflow📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - Check `doc/TASK_PROTOCOL.md` for the verification workflow | |
| - Check `specs/TASK_PROTOCOL.md` for the verification workflow |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@specs/AGENT_CONTEXT.md` at line 193, Update the internal protocol link string
'doc/TASK_PROTOCOL.md' to the correct location 'specs/TASK_PROTOCOL.md' in the
AGENT_CONTEXT.md content so the verification workflow link points to the actual
protocol file; search for the literal 'doc/TASK_PROTOCOL.md' and replace it with
'specs/TASK_PROTOCOL.md' to restore discoverability.
|
|
||
| ``` | ||
| doc/ | ||
| ├── TASK_PROTOCOL.md # This file |
There was a problem hiding this comment.
Directory structure lists an incorrect location for this file.
The doc claims this file is under doc/, but it is actually under specs/.
Suggested fix
-doc/
-├── TASK_PROTOCOL.md # This file
+specs/
+├── TASK_PROTOCOL.md # This file🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@specs/TASK_PROTOCOL.md` at line 30, Update the documentation text in
TASK_PROTOCOL.md that claims the file is located under "doc/" to reflect the
correct location "specs/"; locate the inaccurate directory reference in
TASK_PROTOCOL.md (look for the line or sentence that mentions "doc/") and
replace it with "specs/" and optionally add a brief note clarifying that the
file resides in the specs directory to avoid future confusion.
|
|
||
| 1. Create new task file for the cross-cutting concern | ||
| 2. Add note to original plan: `> **EXTRACTED:** Section moved to X` | ||
| 3. Update TODO.md with the new task |
There was a problem hiding this comment.
Use a consistent TODO path across the protocol.
Elsewhere the document uses tasks/TODO.md; this line drops the tasks/ prefix.
Suggested fix
-3. Update TODO.md with the new task
+3. Update tasks/TODO.md with the new task📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| 3. Update TODO.md with the new task | |
| 3. Update tasks/TODO.md with the new task |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@specs/TASK_PROTOCOL.md` at line 570, Update the inconsistent TODO path:
change the string "Update TODO.md with the new task" to use the same prefixed
path used elsewhere, i.e., "Update tasks/TODO.md with the new task" so the
protocol consistently references tasks/TODO.md.
| @@ -0,0 +1,8 @@ | |||
| # TASK: Replace ALL fmt.Errorf and errors.New in production code with coreerr.E() from go-log. ~8 instances. Import coreerr "forge.lthn.ai/core/go-log". Run tests after. | |||
There was a problem hiding this comment.
Task state appears stale after implementation.
This TODO reads as pending, but the PR itself performs the replacement work. Please mark it completed (or remove it) to avoid reopening already-done work.
Suggested doc update
-**Status:** ready
+**Status:** doneAlso applies to: 4-4, 8-8
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@TODO.md` at line 1, The TODO entry saying "Replace ALL fmt.Errorf and
errors.New in production code with coreerr.E() from go-log" is stale because the
PR already made those replacements; update the TODO by removing that line or
marking it completed (e.g., strike-through or a "Done" note) so it doesn't
reopen work—locate the TODO text fragment "Replace ALL fmt.Errorf and errors.New
in production code with coreerr.E()" in TODO.md and either delete it or annotate
it as completed and run the test suite to confirm nothing else is required.
- Add 'text' language specifier to fenced code blocks in RECENT.md, kb/Rendering.md, and specs/TASK_PROTOCOL.md - Fix heading hierarchy in CONTEXT.md (H1→H3 jump resolved to H1→H2) Co-Authored-By: Virgil <virgil@lethean.io>
…ese are mostly markdown linting i...' (#3) from agent/fix-coderabbit-findings--these-are-mostl into main
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Forge → GitHub Sync
Commits: 1
Files changed: 15
Automated sync from Forge (forge.lthn.ai) to GitHub mirror.
Co-Authored-By: Virgil virgil@lethean.io
Summary by CodeRabbit