-
Notifications
You must be signed in to change notification settings - Fork 2
fix(debug-sqllogger): document third swallowed call site (schema-update path) #48
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,6 @@ | ||
| { | ||
| "name": "indigo", | ||
| "version": "2.1.0", | ||
| "version": "2.1.1", | ||
| "description": "Indigo home automation development toolkit \u2014 plugin development, API integration, and control page building", | ||
| "repository": "https://github.com/simons-plugins/indigo-claude-plugin" | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -91,7 +91,7 @@ Derive the two working paths: | |
|
|
||
| ### Phase 2 — PATCH | ||
|
|
||
| Locate the two call sites in `plugin.py` with `Grep`. Line numbers | ||
| Locate the three call sites in `plugin.py` with `Grep`. Line numbers | ||
| drift across SQL Logger versions — always locate by the message | ||
| fragment, not by number: | ||
|
|
||
|
|
@@ -100,12 +100,19 @@ fragment, not by number: | |
| it's at ~line 529 and already carries `exc_info=True`. | ||
| - Create path — grep `Failed to create table .* for device history`. | ||
| Currently at ~line 476 and does **not** carry `exc_info=True`. | ||
| - Schema-update path — grep `Failed to update schema`. Present in | ||
| SQL Logger 2025.2.0 at ~line 679, inside `_update_device_history`'s | ||
| `(ColumnsMissing, ColumnsChanged)` retry handler; carries | ||
| `exc_info=True`. This path fires when the retried `insert_row` | ||
| after a schema refresh fails (e.g. INT4 overflow) — in a live | ||
| 2026-07 debugging pass it was the **only** site firing, so skipping | ||
| it makes the whole patch cycle come up empty. | ||
|
|
||
| Promote each to `logger.error`, prefix the message with | ||
| `[DEBUG-PATCH] `, and ensure `exc_info=True` is present on both (add | ||
| it to the create call if missing — without it the traceback never | ||
| reaches the log, which defeats the point of the patch). Use `Edit`, | ||
| not `Write`. | ||
| `[DEBUG-PATCH] `, and ensure `exc_info=True` is present on all three | ||
| (add it to the create call if missing — without it the traceback | ||
| never reaches the log, which defeats the point of the patch). Use | ||
| `Edit`, not `Write`. | ||
|
|
||
| Update path before/after: | ||
|
|
||
|
|
@@ -128,13 +135,31 @@ self.logger.debug(f"Failed to create table {table_name} for device history: {err | |
| self.logger.error(f"[DEBUG-PATCH] Failed to create table {table_name} for device history: {err}", exc_info=True) | ||
| ``` | ||
|
|
||
| Schema-update path before/after: | ||
|
|
||
| ```python | ||
| # before (~line 679, with exc_info=True already) | ||
| self.logger.debug(f"Failed to update schema for {dev_table_name}: {schema_err}", exc_info=True) | ||
|
|
||
| # after | ||
| self.logger.error(f"[DEBUG-PATCH] Failed to update schema for {dev_table_name}: {schema_err}", exc_info=True) | ||
| ``` | ||
|
|
||
| If a `grep` finds the fragment but the surrounding arguments differ | ||
| from the above (SQL Logger is maintained; call signatures drift), | ||
| adapt — the invariant is *promote to error, add the DEBUG-PATCH tag, | ||
| ensure exc_info=True*. Every patched line MUST contain the literal | ||
| string `[DEBUG-PATCH]` — the revert step relies on grep returning | ||
| zero hits. | ||
|
|
||
| Completeness check before restarting: grep | ||
| `kErrorKeyDeviceHistoryError` and confirm every `logger.debug` that | ||
| sits next to one of those `_log_error_unobtrusive` calls got | ||
| promoted. SQL Logger is maintained and new swallowed sites appear | ||
| between versions (the schema-update path above shipped after this | ||
| skill was first written). A device-history error path that still | ||
| logs at debug level will produce the Phase 3 symptom below. | ||
|
|
||
| Restart the plugin: | ||
|
|
||
| ``` | ||
|
|
@@ -147,7 +172,19 @@ Ask the user to wait one error cycle (~60s) and signal when ready. | |
| Do not sleep blindly — the cadence varies with server load. | ||
|
|
||
| Read the last 200 lines of the plugin log and search for | ||
| `[DEBUG-PATCH]`. The first matching line names the failing table: | ||
| `[DEBUG-PATCH]`. If the generic error keeps repeating but **no** | ||
| `[DEBUG-PATCH]` line appears after a full cycle, first rule out the | ||
| mundane causes: confirm the plugin actually restarted after the | ||
| patch deployed (grep the plugin log for the restart banner, or check | ||
| the event log for "Started plugin") and that you're reading the | ||
| right log file (`ls -lt` the log directory; rotation can leave you | ||
| tailing yesterday's file). Only once the patched code is confirmed | ||
| live does a missing marker mean a swallowed call site was missed — | ||
| then go back to the Phase 2 completeness check and grep | ||
| `kErrorKeyDeviceHistoryError` for an unpromoted `logger.debug` | ||
| neighbour (this is exactly how the schema-update path was found). | ||
|
coderabbitai[bot] marked this conversation as resolved.
|
||
|
|
||
| The first matching line names the failing table: | ||
|
|
||
| ``` | ||
| [DEBUG-PATCH] Failed to update table device_history_1234567 for device 1234567: integer out of range | ||
|
|
@@ -265,12 +302,13 @@ skill's own extraction output. | |
|
|
||
| ### Phase 6 — REVERT | ||
|
|
||
| Undo every patch. Up to three regions may need reverting: | ||
| Undo every patch. Up to four regions may need reverting: | ||
|
|
||
| 1. `_update_device_history` logger call — restore to | ||
| `self.logger.debug(...)`, remove `[DEBUG-PATCH]` prefix | ||
| 2. `_create_table_for_dev` logger call — same | ||
| 3. `startup()` one-shot DROP block (option c only) — delete the whole | ||
| 2. `_update_device_history` schema-update logger call — same | ||
| 3. `_create_table_for_dev` logger call — same | ||
| 4. `startup()` one-shot DROP block (option c only) — delete the whole | ||
|
Comment on lines
+305
to
+311
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win Update the abort-path safety rule to revert all three logger call sites. Phase 6 now correctly lists the schema-update region, but the Safety Rules below still says to “restore both 🧰 Tools🪛 SkillSpector (2.3.11)[warning] 381: [RA2] Session Persistence: Skill establishes unauthorized persistence across sessions via cron jobs, startup scripts, or state files. Session persistence allows an attacker to maintain access beyond the current interaction. Remediation: Remove any persistence mechanisms (cron jobs, startup scripts, state files). Skills should not maintain state across sessions without explicit user consent. (Rogue Agent (RA2)) 🤖 Prompt for AI Agents |
||
| try/except block | ||
|
|
||
| Verify cleanup with Grep: | ||
|
|
@@ -326,7 +364,7 @@ happening, what to do next. | |
| any point after Phase 2 — user cancels, log-read finds nothing, | ||
| extraction fails, any error, interrupt, or user "none" in Phase 4 — | ||
| the first action before exiting is a full Phase 6 revert | ||
| (restore both `logger.debug` call sites, remove any `startup()` | ||
| (restore all three `logger.debug` call sites, remove any `startup()` | ||
| DROP block, grep-verify zero `[DEBUG-PATCH]` hits, restart plugin). | ||
| A patched `logger.error` left behind will spam the event log every | ||
| ~60s at error level until noticed. | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use
Pluginfor Indigo plugin references.These lines refer to the Indigo Plugin as lowercase “plugin” (
the plugin,plugin log). Capitalize it asPluginto distinguish Indigo Plugins from Claude Code plugin components.As per coding guidelines: “Use 'Plugin' (capitalized) when referring to Indigo plugins.”
🧰 Tools
🪛 SkillSpector (2.3.11)
[warning] 387: [RA2] Session Persistence: Skill establishes unauthorized persistence across sessions via cron jobs, startup scripts, or state files. Session persistence allows an attacker to maintain access beyond the current interaction.
Remediation: Remove any persistence mechanisms (cron jobs, startup scripts, state files). Skills should not maintain state across sessions without explicit user consent.
(Rogue Agent (RA2))
🤖 Prompt for AI Agents
Source: Coding guidelines