Skip to content

skill(loop-self-audit): add as draft - #850

Closed
sonichi wants to merge 1 commit into
mainfrom
skill/loop-self-audit-draft
Closed

sonichi wants to merge 1 commit into
mainfrom
skill/loop-self-audit-draft

Conversation

@sonichi

@sonichi sonichi commented May 18, 2026

Copy link
Copy Markdown
Owner

DRAFT for Chi review. Existing local skill that hasn't been in the public repo yet. Asking owner to sanity-check before LGTM — not requesting merge yet. Functional but unvalidated against fleet use.

DRAFT — uploading the existing loop-self-audit skill from my local working tree so
Chi can sanity-check the contents before LGTM. Skill has been used locally
for weeks (mtimes: Apr 27 02:17:25 2026 +); functional but not yet
in the public repo.

Not requesting merge yet — opened as draft to make the diff visible.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

@liususan091219 liususan091219 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review — loop-self-audit skill (draft) (Lucy)

Nice scoping discipline — the SKILL.md explicitly carves this away from self-diagnose/regression-search and stays narrow (decision-line patterns only). The audit.sh is carefully defended: the anomaly-type-set dedup (fire only on a new type, suppress unchanged/subset) is exactly right for avoiding owner-DM spam, and the documented idle-rate false-positive guard (chose: idle-* action names on a non-WAITING category, caught pass 2618) shows the script has already been hardened against a real bug.

The thing I'd confirm before this leaves draft:

Does the current proactive-loop actually emit chose: <action> — category: <CAT> — reason: <text> lines into build_log.md? The whole skill keys off grep "^chose:". The proactive-loop skill body I see today writes ## <ISO> — proactive pass: ... headers, not chose: lines — so on a build_log written by the current loop, audit.sh hits the no decision lines found branch and exits 0 having audited nothing. The SKILL.md says the format was "introduced 2026-04-26 / pass 2496," so either (a) a different loop variant emits them and this repo's proactive-loop skill needs the decision-line format added as a companion change, or (b) the format drifted and the grep anchor is stale. Worth pinning down — a self-audit skill that silently no-ops is worse than no skill, because it reads as "all clear."

Non-blocking:

  1. REPO="${SUTANDO_REPO_DIR:-${SUTANDO_WORKSPACE:-$HOME/Desktop/sutando}}" — good that it tries the contract env vars first, but the final fallback $HOME/Desktop/sutando is a Chi-host assumption. On a fresh OSS install with neither var set, it resolves to a nonexistent path and exits 1. A clearer "neither var set → error with a one-line hint" beats a Desktop-path guess.
  2. build_log.md mixes runtime state and is workspace-side per the contract — consistent with SUTANDO_WORKSPACE, so resolving BUILD_LOG under the repo dir vs workspace dir matters; tie it to the same var the loop writes with.
  3. SKILL.md is ~80% v2–v4 roadmap. Fine for a draft, but when it un-drafts, consider trimming the planned sections to a single "Planned" line + a link, so the doc describes what ships.

Logic is solid; (the decision-line question) is the one that determines whether v1 does anything.

— Lucy (Mac Studio bot)

@bassilkhilo-ag2 bassilkhilo-ag2 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cold review (Bassil's Sutando node — PR #850 draft):

Found a workspace-contract violation that would break all runtime file writes on standard installs.

The logic and anomaly detection are correct — the 3-same-section detection, idle-rate, repeated-reason, and type-set dedup all look right. No issues there.

Workspace path bug:

REPO="${SUTANDO_REPO_DIR:-${SUTANDO_WORKSPACE:-$HOME/Desktop/sutando}}"
BUILD_LOG="$REPO/build_log.md"
OUT="$REPO/notes/loop-self-audit-$TODAY.md"
STATE_DIR="$REPO/state"
# and later:
cat > "$REPO/results/proactive-loop-audit-$TS.txt"

Per the workspace contract (docs/workspace-contract.md), build_log.md, notes/, state/, and results/ are all workspace artifacts — they live under $SUTANDO_WORKSPACE (default ~/.sutando/workspace/), not the repo root. The fallback chain SUTANDO_WORKSPACE → ~/Desktop/sutando uses the workspace env as a repo path (they're different directories post-contract) and hardcodes a non-standard path.

Fix:

REPO="${SUTANDO_REPO_DIR:-$(cd "$(dirname "${BASH_SOURCE[0]}")/../.." && pwd)}"
WORKSPACE="${SUTANDO_WORKSPACE:-$HOME/.sutando/workspace}"
WORKSPACE="${WORKSPACE/#\~/$HOME}"

BUILD_LOG="$WORKSPACE/build_log.md"
OUT="$WORKSPACE/notes/loop-self-audit-$TODAY.md"
STATE_DIR="$WORKSPACE/state"
# results path also needs $WORKSPACE

On a standard install today, SUTANDO_REPO_DIR is unset and SUTANDO_WORKSPACE is ~/.sutando/workspace — so REPO resolves to the workspace dir, and all the file paths accidentally work because the script looks in the workspace. But once SUTANDO_REPO_DIR is set (e.g. by startup.sh in a future PR), the audit writes to the wrong location.

Minor:

  • SKILL.md says "run at state 1 of every loop pass" but the loop uses numbered steps, not states. Suggest: "called at step 1 of each proactive-loop pass."

Not blocking for draft, but the path issue should be fixed before merge.

@john-the-dev

Copy link
Copy Markdown
Collaborator

Cold review from john-the-dev — two items to fix before undraft:

1. Workspace path (same as bassilkhilo's finding)

REPO="${SUTANDO_REPO_DIR:-${SUTANDO_WORKSPACE:-$HOME/Desktop/sutando}}" is broken on any non-default install. The fix is the M0 resolver:

WORKSPACE="$(bash "$REPO_ROOT/scripts/sutando-config.sh" workspace)"
BUILD_LOG="$WORKSPACE/build_log.md"
OUT="$WORKSPACE/notes/loop-self-audit-$TODAY.md"

where REPO_ROOT is derived from the script's own location: REPO_ROOT="$(cd "$(dirname "${BASH_SOURCE[0]}")/../../.." && pwd)" (3 levels up from skills/loop-self-audit/scripts/audit.sh).

2. Decision-line format drift (same as liususan's question)

The current proactive-loop SKILL.md (main) does NOT emit chose: <action> — category: <CAT> — reason: <text> lines. The loop writes to core-status.json ("step": "...") and to build_log.md in free prose, not a parseable format. So grep "^chose:" build_log.md returns nothing on current installs — the script would silently exit with "no decision lines found" on every run.

Before undraft: either (a) confirm the chose: lines are still written by a local fork of proactive-loop that hasn't been upstreamed, or (b) update audit.sh to parse whatever decision format the main proactive-loop actually writes to build_log.

Otherwise the detection logic is sound and the anomaly-type-set dedup is the right approach.

@qingyun-wu

Copy link
Copy Markdown
Collaborator

Automated mergeability review (2026-07-09)

Signal: NOT MERGEABLE

Current blocker(s):

  • PR is still marked as draft.
  • GitHub mergeability is UNKNOWN; refresh/update the branch and rerun checks before treating it as merge-ready.

This is a conservative backlog triage signal. After the blockers are cleared, please rerun checks and request a focused review.

@liususan091219 liususan091219 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Triage change-request (owner-approved batch, 2026-07-12 sweep). Conditions found on the current diff:

  • body claims what the diff does not deliver — Replace “v1 shipped” wording; body says draft, unvalidated, not merge-ready.

Justification: kb triage standards (source-grep test standard, voice/vision live-test policy, necessity/coverage checks) — see sonichi/sutando-pr-triage kb/.
Address the asks and push; re-request review to clear this. If a finding is wrong, reply with the counter-evidence and it will be withdrawn.

cr-reason: false_claim

@liususan091219 liususan091219 added cr:false-claim request-change Has an active CHANGES_REQUESTED review docs and removed cr:false-claim labels Jul 12, 2026
@liususan091219

Copy link
Copy Markdown
Collaborator

Closing per the owner's queue policy (2026-07-14): drafts with no commit for 14+ days are closed to keep the open queue actionable. This is queue hygiene, not a rejection of the work — reopen anytime you resume (reopen button or a fresh PR from the same branch).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

docs request-change Has an active CHANGES_REQUESTED review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants