docs(statusline): document EXIT trap contract and STATUSLINE_SCRIPT coupling

[nsheaps]

Summary

Address follow-up items from Henry's PR #164 review:

• EXIT trap contract: Document that safe_write_settings sets and clears an EXIT trap internally. Callers must not rely on their own EXIT traps surviving a call to this function. Both success and error paths clear the trap before returning.
• STATUSLINE_SCRIPT coupling: Document the global variable coupling as a known limitation with a TODO for refactoring to parameter-based jq args when a 3rd consumer appears.

Test plan

• Verify header comments are clear and accurate
• No functional changes — documentation only

🤖 Generated with Claude Code

[henry-nsheaps]

❌ EXIT trap documentation describes behavior that no longer exists — must be fixed before merge

❌ EXIT trap contract docs describe phantom behavior (lines 20-26 of safe-settings-write.sh)
⚠️ Version bump still at 0.1.0 — needs minor bump for new skill
⚠️ git log --oneline redundancy and command-description mismatch in SKILL.md
❔ PR description doesn't mention 2 of the 4 changed files (daily-summary-report skill + session-telemetry spec)

🖱️ Click to expand for full details

Core Issue: Stale EXIT Trap Documentation

The PR's stated primary purpose — "Document that safe_write_settings sets and clears an EXIT trap internally" — is now documenting behavior that does not exist.

The original commit (335ab95, Feb 18) was accurate at the time. However, subsequent PRs merged to main fundamentally changed the function:

• PR #196 (abee0fc): Replaced temp file logic with direct write
• PR #207 (047ee2f): Simplified to naïve jq approach

These removed all locking, lockdir, trap, rmdir, and mktemp logic. The merge commit 48aee39 brought the simplified code into this branch, but the documentation was not updated. Lines 20-26 now describe a trap contract for a function that has no traps.

Quality score rationale (55%): The primary deliverable of this PR is incorrect documentation. While the STATUSLINE_SCRIPT coupling docs (lines 28-33) are accurate, and the new skill + spec are well-written, the core stated purpose of the PR actively misleads readers. This pulls the quality score significantly below passing.

Version Bump

Plugin version is 0.1.0 on both this branch and main. Adding a new skill (daily-summary-report) with description/keyword updates is a feat warranting at least a minor bump to 0.2.0. This has been flagged since the first review cycle. CD auto-bump handles patch only.

SKILL.md Git Command Issues

Two related issues in the daily-summary-report skill:

1. Data Sources table (line 230): git log --all --oneline claims to extract "Commits, authors, timestamps" but --oneline only produces abbreviated hashes + subjects
2. Gathering Commits code block (line 243): --oneline is a no-op when --format is specified

PR Scope vs Description

The PR description only mentions the safe-settings-write.sh documentation changes, but the PR also includes:

• 289-line daily-summary-report skill (new file)
• 494-line session-telemetry draft spec (new file)
• plugin.json description/keyword updates

These are substantial additions that should be reflected in the PR summary. Consider either updating the description or splitting into focused PRs.

What's Good

• ✅ STATUSLINE_SCRIPT coupling documentation (lines 28-33) is accurate and well-written
• ✅ Daily summary report skill is comprehensive with clear lifecycle model
• ✅ Session telemetry spec is thorough with good open questions section
• ✅ Simplicity score (90%) — all new files are well-organized and clearly structured

Recommended follow-ups (non-blocking):

• The daily-summary-report skill references ~/Documents/ as output location — consider adding a note about portability for CI/container/web sessions
• Session telemetry spec should be mentioned in the PR description or split into its own PR for independent review

Notes:¹²

Footnotes

1. Workflow run: https://github.com/nsheaps/ai-mktpl/actions/runs/24149864565/attempts/1 ↩

2. PR: nsheaps/ai-mktpl#165 ↩

[henry-nsheaps]

❌ EXIT trap contract documentation is stale and incorrect — unchanged since last review.

The function was simplified on main by PRs #196 (abee0fc) and #207 (047ee2f), removing all locking, lockdir, trap, rmdir, and mktemp logic. After the merge commit 48aee39 brought main into this branch, the function is now a simple 29-line jq wrapper:

safe_write_settings() {
  local jq_filter="$1"
  # ensure file exists → build jq args → jq | capture → write
}

There are no EXIT traps, no locks, no atomic renames in the current implementation. These 7 lines describe behavior that no longer exists and will actively mislead consumers of this library.

Fix: Remove the EXIT trap block entirely and optionally note the simplification history:

Suggested change

	#
	# EXIT trap contract: safe_write_settings sets and clears an EXIT trap
	# internally for lock cleanup. Callers must not rely on their own EXIT
	# traps surviving a call to this function — any previously set EXIT trap
	# will be overwritten. Both success and error paths clear the trap via
	# `trap - EXIT` before returning, so subsequent caller traps set after
	# the call will work normally.
	#
	# Note: This function previously used mkdir-based locking and EXIT traps
	# for concurrent-safe writes. That complexity was removed in PRs #196
	# and #207 (simplified to a direct jq read → variable capture → file
	# write with no locking). No special caller considerations are needed.
	#

See also: prior thread with same finding

[henry-nsheaps]

⚠️ Version bump needed — flagged in 8+ prior review cycles, still at 0.1.0.

Adding a new skill (daily-summary-report) plus description/keyword updates is a feat warranting at least a minor bump. CD auto-bump only does patch increments, so a manual bump to 0.2.0 is recommended.

Suggested change

	"version": "0.1.0",
	"version": "0.2.0",

Note: Per versioning rules, CD will catch this as a patch bump if not done manually — so non-blocking, but a minor bump better reflects the change scope.

[henry-nsheaps]

⚠️ Command-description mismatch: git log --all --oneline only produces abbreviated hashes and subject lines. It does not extract "authors" or "timestamps" as the "What to extract" column claims.

Fix the command to match the description:

Suggested change

	| `git log --all --oneline` (per repo) | Commits, authors, timestamps |
	| `git log --all --format="%H|%aI|%an|%s"` (per repo) | Commits, authors, timestamps |

This is consistent with the "Gathering Commits" code block on line 243 which already uses --format.

[henry-nsheaps]

⚠️ --oneline is redundant with --format: The --format flag completely overrides --oneline's output, making --oneline a no-op here.

Suggested change

	git log --oneline --since="YYYY-MM-DDT00:00:00" --until="YYYY-MM-DDT23:59:59" --format="%H|%aI|%s"
	git log --since="YYYY-MM-DDT00:00:00" --until="YYYY-MM-DDT23:59:59" --format="%H|%aI|%s"

[henry-nsheaps]

❔ Not mentioned in PR description: This 494-line draft spec is a significant addition but isn't referenced in the PR summary at all. The PR title and body only describe the safe-settings-write.sh documentation changes.

Consider updating the PR description to mention both the daily-summary-report skill and this spec, or splitting them into separate PRs for clearer review scope. As a draft spec in docs/specs/draft/, the content itself looks thorough and well-structured — the open questions section is particularly useful.

[henry-nsheaps]

✅ STATUSLINE_SCRIPT coupling documentation is accurate. The function still checks ${STATUSLINE_SCRIPT:-} at line 46 to conditionally pass --arg script to jq. The TODO trigger ("3rd consumer") is pragmatic YAGNI, and the suggested API refactoring is concrete and actionable.