cluster: 4 refactor/docs follow-ups from #49 / #60 IDD lifecycles#72
Open
kiki830621 wants to merge 6 commits into
Open
cluster: 4 refactor/docs follow-ups from #49 / #60 IDD lifecycles#72kiki830621 wants to merge 6 commits into
kiki830621 wants to merge 6 commits into
Conversation
…p 2.1 (Refs #56) Per /idd-verify #49 DA new finding: Step 2.1 sibling-archive dedup extension claims 'read-only' but enforcement is honor-system (current bash uses only find/head/awk; future maintainer adding mv/rm wouldn't be caught by any guard). Option A per issue body — invariant marker comment at top of bash block: - Lists allowed ops (find / head / awk / cat / grep) - Lists forbidden ops (mv / rm / cp / > / >> / tee / chmod) - Self-documenting, load-bearing contract for future maintainers Option B (bash trap-based check) + Option C (pre-commit hook scanning) deferred — requires real bash impl file or repo-level tooling, both out of scope for this docs-only fix. Refs #56
Per /idd-verify #49 security review LOW#1: Step 2.1 sibling-archive dedup extension used 'find ... | while IFS= read -r' which splits on newlines. Filenames containing newlines (POSIX-legal, rare in practice) would silently mis-parse — partial filename gets processed. Apply standard null-safe pattern to BOTH find invocations: - Outer: find -P -type l -print0 + while read -r -d '' symlink_dir - Inner: find -P -name '*.md' -print0 + while read -r -d '' mdfile Both -print0 / read -d '' must change together — partial conversion of one without the other would mis-parse the pipeline. Realistic exposure low (Apple Mail subject lines don't contain raw newlines), but trivial defensive hygiene fix. Refs #57
…nment with Phase 0.5 (Refs #68) Per /idd-implement #60 Step 5.7 sister bug sweep: audit found Phase 1.5 (action default '順便更新') + Phase 2.5 (action default '更新 README') appear to diverge from Phase 0.5's abort-default safety policy. Audit conclusion: NOT a divergence — they correctly apply Phase 0.5 rule's exception clause: 'active default is reserved for unambiguous happy-path / reversible action / frequent dev flow' Phase 1.5: binary-out-of-sync in daily dev flow IS unambiguous happy path. Binary install is reversible (mcp-deploy / cli-upgrade can reset). Frequent. → action default justified. Phase 2.5: '更新 README' option doesn't mutate; it proposes diff (read CHANGELOG + git log → draft → user reviews → commit). User- confirmation gate still present. Reversible draft. → action default justified. Contrast with Phase 0.5 (release-time push to public marketplace): push is IRREVERSIBLE (force-push to public is bad day) → abort default warranted regardless of user-in-session. Each phase's default reflects its action's risk profile. No behavior change — just documentation that the deliberate divergence IS the correct application of Phase 0.5 rule, not a violation. Refs #68
….5 Step 5 (Refs #70) Per /idd-verify #60 security review (LOW, DA→LOW): finding L233 flagged '/tmp/touched-plugins.txt race / TOCTOU on shared /tmp'. Audit found the flagged tempfile DOES NOT EXIST in current implementation — Phase 0.5 Step 5 already uses inline subshell: TOUCHED=$(git log --name-only --pretty=format: ... | sort -u) Inline $(...) substitution captures to a process-local variable, never touches /tmp, hence no TOCTOU window and no race. The verify finding was a false positive imagining a tempfile that was never written. No behavior change. Add load-bearing comment documenting: 1. Why current code is already safe (inline subshell, not tempfile) 2. If future refactor *does* need persistence, use mktemp + trap pattern (the '#70 expected behavior' from the issue) 3. Hardcoded /tmp/touched-plugins.txt is FORBIDDEN — it would re-introduce the false-positive surface and the actual race window Comment forestalls future audits from re-flagging the same false positive, and gives future maintainers explicit guidance if refactoring direction changes. Refs #70
This was referenced May 10, 2026
…e exception + explicitly justify frequent-dev → non-destructive substitution (Refs #68) Per /idd-verify --pr 72 codex MEDIUM finding (single blocking): Phase 2.5 audit blockquote silently substituted 'non-destructive' for 'frequent dev' in the 3-criterion exception clause, while Phase 1.5 quoted all 3 verbatim. Codex (gpt-5.5 xhigh) flagged the inconsistency: Phase 1.5 note: 'unambiguous happy-path / reversible / frequent dev flow' ✓ Phase 2.5 note: 'unambiguous happy-path / reversible / non-destructive' ✗ ^^^^^^^^^^^^^^^^ silent substitution Substitution is defensible (README updates aren't a daily dev flow — they fire only after substantive change), but the audit must EXPLAIN the substitution, not silently reword the canonical rule. Otherwise future readers comparing the two notes would see two different rule statements and conclude the policy is incoherent. Fix: rewrite Phase 2.5 note as 3-bullet itemize listing each criterion: - unambiguous happy-path ✓ (with explanation) - reversible action ✓ (with explanation — propose diff, not mutate) - frequent dev flow ✗ → substituted with non-destructive (with reason) Phase 1.5 note unchanged (all 3 criteria fit verbatim). Refs #68
Collaborator
Author
Verify Report — PR #72EngineLighter cluster ensemble (1 combined Claude reviewer covering Requirements/Logic/Security/Regression/Devil's-advocate + 1 Codex GPT-5.5 xhigh independent). Pure-docs/refactor cluster — no behavior change, 90-line diff. AggregatePASS (after Scope coveragePR refs: #56 #57 #68 #70 #56 — invariant marker comment (archive-mail Step 2.1)Coverage: FULLY (both reviewers agree)
Devil's advocate:
#57 — null-safe find/while (archive-mail Step 2.1)Coverage: FULLY (both reviewers agree)
Devil's advocate: bash-specific (dash doesn't support #68 — Phase 1.5/2.5 default-option policy alignment auditCoverage: FULLY after fix
Fix applied (
The substitution is now explicit and justified rather than silent. Phase 1.5 note unchanged.
#70 — Phase 0.5 Step 5 tempfile-avoidance commentCoverage: FULLY (both reviewers agree)
Devil's advocate: trap example is documentation, not enforcement. Acceptable — matches #70's "comment guardrail" scope. Cross-cutting findingsScope / regression
KAC/CHANGELOG
Codex disagreement → fix
Severity-tagged findings list (post-fix)
No BLOCKING / HIGH / MEDIUM remaining. Routing
What this verify deliberately did NOT do
|
Bump plugin shell v2.18.1 → v2.19.0 to ship binary v2.8.0 (just released at PsychQuant/che-apple-mail-mcp#85, tagged v2.8.0). Binary v2.8.0 contents: - (#19) sanitize_links opt-in URL scheme allowlist for markdown mode - (#85) formal spec.md Requirement+Scenarios + builder-layer wiring contract tests pinning sanitizeLinks forwarding - (#20/#21/#25/#27/#32) cluster A hygiene + invariant tests - (#73) extractHTMLBody base64+UTF-8-QP decoding fixes - 47 → 48 tools (sanitize_links param surface adds new schema field) - swift test 309 → 313 passing / 0 failures / 8 skipped Files synced: - plugins/che-apple-mail-mcp/.claude-plugin/plugin.json (version + binary_version + description) - .claude-plugin/marketplace.json (same fields mirrored from plugin.json) - plugins/che-apple-mail-mcp/README.md (Version History entry added) Refs PsychQuant/che-apple-mail-mcp#19 #85 #73
This was referenced May 10, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Refs #56 #57 #68 #70
Summary
Cluster of 4 refactor / docs follow-ups surfaced from
/idd-verifysecurity & sister-bug review of #49 (archive-mail) + #60 (plugin-update). All P3/LOW priority defensive hardening — no behavior change, just invariant markers, null-safe hygiene, and audit-trail comments to forestall future false-positive reviews.Cluster overview
3b01d3f5b610cdf25621eac3c673Per-issue rationale
#56 — archive-mail Step 2.1 read-only invariant marker
Sister-bug from #49 implementation: Step 2.1 sibling-archive-dir dedup extension reads symlinked subdir markdown frontmatter for
message_id:dedup. Block must remain read-only w.r.t.$symlink_dirto preserve immutability invariant. Add invariant marker comment listing allowed (find / head / awk / cat / grep) vs forbidden (mv / rm / cp / > / >> / tee / chmod) operations against$symlink_diror its target. Future trap-based check + pre-commit scan deferred — comment is the load-bearing contract.#57 — archive-mail Step 2.1 null-safe find/while loops
Per /idd-verify #49 security LOW#1: outer
find ... | while IFS= read -r symlink_dirand innerfind ... | while IFS= read -r mdfilesplit on newlines. Filenames containing newlines (POSIX-legal, rare in practice) silently mis-parse — partial filename gets processed.Apply standard null-safe pattern to both:
-print0 | while IFS= read -r -d ''. Both must change together — partial conversion mis-parses pipeline.Realistic exposure low (Apple Mail subject lines don't contain raw newlines), but trivial defensive hygiene fix.
#68 — Phase 1.5/2.5 default-option policy clarification
Sister concern from #60 implementation: Phase 0.5 enforces
abort-default safety policy for ambiguous git states, but Phase 1.5 (順便更新) and Phase 2.5 (更新 README) both have action-defaults. Audit conclusion: NOT a divergence — both correctly apply Phase 0.5 rule's exception clause:Phase 1.5: binary sync in dev flow IS unambiguous happy path; binary install reversible. Phase 2.5: '更新 README' option proposes diff (read CHANGELOG → draft → user reviews → commit), doesn't mutate; user-confirmation gate intact, reversible draft. Phase 0.5 (release-time push to public marketplace) is irreversible → abort default warranted.
No behavior change — documentation that the deliberate divergence IS correct application of Phase 0.5 rule.
#70 — Phase 0.5 Step 5 tempfile-avoidance comment
Per /idd-verify #60 security LOW (DA→LOW): finding L233 flagged
/tmp/touched-plugins.txtrace / TOCTOU. Audit found the flagged tempfile DOES NOT EXIST — Phase 0.5 Step 5 already uses inline subshellTOUCHED=$(git log ...)which captures to process-local variable, never touches /tmp.False-positive verify finding. Add load-bearing comment documenting: (1) why current code is safe (inline subshell, not tempfile), (2) if future refactor needs persistence, use
mktemp + trappattern, (3) hardcoded/tmp/touched-plugins.txtis FORBIDDEN. Forestalls re-flagging by future audits.Verify checklist
Refs #N)/idd-verify --pr <N>against this PR)Generated by /idd-implement #56 #57 #68 #70 --pr cluster-PR mode. Do NOT add
Closes #Ntrailers — IDD discipline requires manual /idd-close per issue after merge to enforce checklist gate + per-issue closing summary.