mass-update: deterministic update - #8
Open
OriNachum wants to merge 1 commit into
Open
Conversation
|
|
|
PR Summary by QodoAdd devague workflow skills and an ask-colleague CLI wrapper
AI Description
Diagram
High-Level Assessment
Files changed (15)
|
Code Review by Qodo
1. Skill dirs missing scripts/
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+65
to
+69
| ## Method-only — no script, no CLI verb | ||
|
|
||
| This is a **method-only** skill (v1), modelled on `/scope` at its birth: a | ||
| `SKILL.md` with an output template and no entry-point script, no new CLI verb. | ||
| The deterministic devague CLI surface is **unchanged** — Devague never spawns |
There was a problem hiding this comment.
1. Skill dirs missing scripts/ 📜 Skill insight ⌂ Architecture
This PR adds new/updated skills that do not include a scripts/ directory with an executable entry-point, which violates the required skill directory structure. Without an entry-point script, these skills cannot be invoked consistently and do not meet the repository’s skill packaging contract.
Agent Prompt
## Issue description
Several skills added/modified in this PR do not include a `scripts/` directory with an executable entry-point script, but the compliance rule requires every skill directory to have `SKILL.md` *and* `scripts/` with an entry-point.
## Issue Context
`.claude/skills/summarize-delivery/SKILL.md` explicitly documents itself as having “no entry-point script”, which directly conflicts with the requirement.
## Fix Focus Areas
- .claude/skills/summarize-delivery/SKILL.md[65-80]
- .claude/skills/challenge/SKILL.md[1-25]
- .claude/skills/deviate/SKILL.md[1-30]
- .claude/skills/scope/SKILL.md[1-30]
(Implement by adding `scripts/` directories and at least one executable entry-point script per skill, using the portable bash shebang + `set -euo pipefail`.)
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+430
to
+477
| ASK_COLLEAGUE_REAL_ARTIFACT_DIR="${1:-}" ASK_COLLEAGUE_GRADABLE="${2:-}" ASK_COLLEAGUE_DRIVE_RC="${3:-}" ASK_COLLEAGUE_JSON="${JSON_OUT:-0}" ASK_COLLEAGUE_MAX_STEPS="${MAX_STEPS:-20}" python3 -c ' | ||
| import sys, json, os | ||
| raw = sys.stdin.read().strip() | ||
| json_mode = os.environ.get("ASK_COLLEAGUE_JSON") == "1" | ||
| def _fail(code, message, remediation, detail=None): | ||
| # #226: in --json mode emit a structured {code, message, remediation} object | ||
| # on stderr (matching colleague CliError shape) so a machine consumer parses | ||
| # a failure the same way it parses success; in text mode keep the standard | ||
| # error:/hint: contract. stdout stays clean (no result) on every path. | ||
| if json_mode: | ||
| obj = {"code": code, "message": message, "remediation": remediation} | ||
| if detail is not None: | ||
| obj["detail"] = detail | ||
| sys.stderr.write(json.dumps(obj) + "\n") | ||
| else: | ||
| sys.stderr.write("error: " + message + "\n") | ||
| if detail is not None: | ||
| sys.stderr.write(detail + "\n") | ||
| if remediation: | ||
| sys.stderr.write("hint: " + remediation + "\n") | ||
| sys.exit(code) | ||
| if not raw: | ||
| _fail(2, "colleague produced no result on stdout (see diagnostics above)", | ||
| "re-run; if it persists, check the backend with colleague doctor --probe") | ||
| try: | ||
| d = json.loads(raw) | ||
| except Exception: | ||
| _fail(2, "could not parse colleague --json output", | ||
| "the backend may have emitted non-JSON; see the raw output and diagnostics above", | ||
| detail=raw[:2000]) | ||
| ok = d.get("status") == "ok" | ||
| inc = d.get("incompletion") if isinstance(d.get("incompletion"), dict) else None | ||
| tid = d.get("task_id") or "" | ||
| # Resolve the artifact path to the preserved copy when the drive ran in a | ||
| # throwaway worktree (read-only verbs); the raw JSON points into the now-deleted | ||
| # worktree, so both the digest and the --json output report the real location. | ||
| ap = d.get("artifacts_path") | ||
| real_dir = os.environ.get("ASK_COLLEAGUE_REAL_ARTIFACT_DIR") or "" | ||
| if ap and real_dir: | ||
| ap = os.path.join(real_dir, os.path.basename(ap)) | ||
| # A drive that stopped without calling finish (colleague#142) or exhausted its | ||
| # step budget did NOT deliver an authoritative result — its summary is the model | ||
| # trailing off mid-task. Warn so the caller treats it as a partial, not a verdict. | ||
| # The warning is a DIAGNOSTIC -> always stderr (never stdout), so both the digest | ||
| # and --json keep a clean, machine-readable stdout (no single quotes in this body). | ||
| # Enriched partial warnings: include reached step count and a concrete larger | ||
| # --max-steps to retry with, so the hint is actionable (#194). | ||
| max_steps = int(os.environ.get("ASK_COLLEAGUE_MAX_STEPS", "20")) |
There was a problem hiding this comment.
2. Max-steps parse crash 🐞 Bug ☼ Reliability
ask-colleague.sh accepts an arbitrary string for --max-steps and later unconditionally converts it with Python int() while rendering results, so a non-numeric value crashes result rendering (ValueError) and can hide an otherwise valid colleague run output.
Agent Prompt
### Issue description
`--max-steps` is stored as a raw string and later parsed with `int(...)` inside `print_result`’s embedded Python. If a user passes a non-integer value, result rendering crashes.
### Issue Context
This is a CLI wrapper: malformed CLI input should produce a controlled, user-input error (exit 1 + clear message), not a Python traceback or aborted rendering.
### Fix Focus Areas
- .claude/skills/ask-colleague/scripts/ask-colleague.sh[203-244]
- .claude/skills/ask-colleague/scripts/ask-colleague.sh[430-478]
### Proposed fix
- When parsing `--max-steps`, validate it matches `^[0-9]+$` and is > 0; otherwise print `error: --max-steps must be a positive integer` and `exit 1`.
- (Optional hardening) Wrap the embedded Python `int(...)` conversion in `try/except` and fall back to 20 if somehow invalid, to prevent rendering failures from any unexpected input source.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+820
to
+832
| front_load_review_diff() { | ||
| local cap="${COLLEAGUE_MAX_OUTPUT_CHARS:-100000}" | ||
| printf '%s\n' "--- DIFF UNDER REVIEW (filtered + capped; read specific files for more) ---" | ||
| git -C "$REPO" diff "$BASE"...HEAD --stat | ||
| printf '\n' | ||
| local diff_body | ||
| diff_body="$(git -C "$REPO" diff "$BASE"...HEAD -- . ':(exclude)*.lock' ':(exclude)**/*.lock' ':(exclude)package-lock.json' ':(exclude)**/package-lock.json' ':(exclude)*.min.js' ':(exclude)**/*.min.js' 2>/dev/null || true)" | ||
| if [[ "${#diff_body}" -gt "$cap" ]]; then | ||
| printf '%s\n' "${diff_body:0:$cap}" | ||
| printf '%s\n' "[... diff body truncated at ${cap} chars; read specific files for the rest ...]" | ||
| else | ||
| printf '%s\n' "$diff_body" | ||
| fi |
There was a problem hiding this comment.
3. Invalid diff cap crash 🐞 Bug ☼ Reliability
front_load_review_diff() uses COLLEAGUE_MAX_OUTPUT_CHARS directly in bash numeric comparison and substring slicing; if it is set to a non-integer, the review path can terminate with an arithmetic/substitution error under set -e.
Agent Prompt
### Issue description
`COLLEAGUE_MAX_OUTPUT_CHARS` is treated as an integer but is not validated. A non-numeric value can break `[[ ... -gt ... ]]` and `${diff_body:0:$cap}` causing the `review` verb to fail.
### Issue Context
This value is environment/config-driven; failures here are confusing and prevent `ask-colleague review` from running.
### Fix Focus Areas
- .claude/skills/ask-colleague/scripts/ask-colleague.sh[816-833]
### Proposed fix
- Validate `cap` with a numeric regex before using it:
- If invalid (or <= 0), either:
- print a warning to stderr and fall back to `100000`, or
- print an error to stderr and exit 2 (config/env error), depending on desired contract.
- Ensure the validated `cap` is used in both the `-gt` comparison and substring slice.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
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.
Automated update via mass-update skill.