feat(build-server): add client+server interaction audit logging#949
Merged
cuioss-oliver merged 15 commits intoJul 20, 2026
Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sorry @cuioss-oliver, you have reached your weekly rate limit of 500000 diff characters.
Please try again later or upgrade to continue using Sourcery
|
Warning Review limit reached
Next review available in: 38 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository: cuioss/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (16)
📝 WalkthroughWalkthroughThe change adds append-only daemon interaction auditing, client work-log entries for submit and wait outcomes, job correlation across logs and the ledger, control-character sanitization, project-scoped audit inspection, retention cleanup, and deterministic test/tooling updates. ChangesInteraction audit logging
Repository hygiene and deterministic tests
Estimated code review effort: 4 (Complex) | ~45 minutes 🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a central, append-only interaction audit logging system for the marshalld build server. It logs every ping, submit, and wait request to a central interaction-audit.log file, ensuring accountability while maintaining strict secrets discipline by excluding sensitive fields like raw command arguments. Additionally, a new read-only logs verb is added to manage_build_server to allow project-scoped inspection of these logs. Comprehensive unit and integration tests have been added to verify the audit logging, log rotation/GC, and security constraints. No review comments were provided, so there is no feedback to address.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
cuioss-oliver
force-pushed
the
feature/marshalld-interaction-audit-logging
branch
from
July 20, 2026 06:24
fa9bafd to
596e404
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@marketplace/bundles/plan-marshall/skills/build-server-client/scripts/build_server.py`:
- Around line 466-472: Sanitize the job_id value in the run_wait audit logging
flow before interpolating it into the _audit_log message, applying the same
newline/carriage-return control-character stripping already used for notation.
Preserve the existing job-status, elapsed, and eta formatting.
In
`@marketplace/bundles/plan-marshall/skills/manage-build-server/scripts/marshalld.py`:
- Around line 313-326: Move the _audit_attribution, _audit_job_id, and reason
derivation into the existing try block so all audit-related operations are
covered by the best-effort guard. Keep catching OSError and swallowing it,
ensuring failures from attribution resolution or _interaction_audit.record never
abort request handling.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: cuioss/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: ef7fa657-14d8-447d-ad83-92fe90ab643f
📒 Files selected for processing (12)
marketplace/bundles/plan-marshall/skills/build-server-client/SKILL.mdmarketplace/bundles/plan-marshall/skills/build-server-client/scripts/build_server.pymarketplace/bundles/plan-marshall/skills/manage-build-server/SKILL.mdmarketplace/bundles/plan-marshall/skills/manage-build-server/scripts/_marshalld_audit.pymarketplace/bundles/plan-marshall/skills/manage-build-server/scripts/manage_build_server.pymarketplace/bundles/plan-marshall/skills/manage-build-server/scripts/marshalld.pytest/plan-marshall/build-server/test_build_server_client.pytest/plan-marshall/build-server/test_interaction_audit_correlation.pytest/plan-marshall/build-server/test_manage_build_server.pytest/plan-marshall/build-server/test_marshalld_audit.pytest/plan-marshall/script-shared/test_build_execute_factory.pytest/plan-marshall/script-shared/test_build_queue_slot.py
cuioss-oliver
added a commit
to cuioss/cuioss-organization
that referenced
this pull request
Jul 20, 2026
…rol (#188) Two changes to `reusable-pyprojectx-verify.yml`, shipped together. ## (A) Structured build-target inputs `reusable-maven-build.yml` gives callers named, validated levers over the build's content (java-versions, maven-profiles-*, enable-sonar, enable-snapshot-deploy). The pyprojectx equivalent had only `verify-command` — a raw string interpolated straight into a `run:` step. Changing the goal set meant hand-writing `'./pw quality-gate && ./pw module-tests'`: unvalidated, not composable, no meaningful per-repo defaults. Replaced with: | Input | Default | Purpose | |---|---|---| | `verify-goals` | `verify` | pw goals run in order, each as `./pw <goal>` | | `verify-args` | `''` | extra args appended to every goal invocation (the profile-equivalent knob, e.g. a module filter) | Goal tokens are validated against `[a-z][a-z0-9-]*` and the run fails fast with a clear annotation otherwise. Both values are carried via `env` and never interpolated into the shell body. **One deviation from the request:** it asked for each goal to be *its own step* with an attributable step name. GitHub Actions cannot emit a dynamic number of steps from a runtime string — steps are static YAML. The goals therefore run in a single step with a `::group::` per goal; the loop exits on the first failure and emits `::error::pw goal 'X' failed`, giving the same attribution a step name would. The alternative (a fixed static step per known goal) was rejected because it freezes the goal set in the reusable and loses caller-specified ordering. ## (B) Concurrency control `reusable-pyprojectx-verify.yml` had no `concurrency:` block. Without it a push and its pull_request event both ran a full verify on the same commit and neither was cancelled — observed in cuioss/plan-marshall#949 as two concurrent `verify / verify` jobs grinding on one SHA for ~60 minutes. The added block is semantically identical to `reusable-maven-build.yml`: grouped by caller workflow + ref + event, `cancel-in-progress` limited to `pull_request` so default-branch pushes and merge_group (merge-queue) runs always run to completion. This is **complementary to, not overlapping with, the gate job**: - the gate skips a **redundant** run (a push whose commit an open PR already covers) - concurrency cancels a **superseded** run (an older commit on the same PR) `merge_group` runs are excluded from both — the gate always builds them, and they are never cancelled. The gate's footprint filter and push-dedup logic are otherwise untouched. ## Incidental fix `schema.json` had a pre-existing trailing comma (in `github-automation`) that made it **invalid JSON**, silently breaking the published `yaml-language-server: $schema` editor hint for every consumer. Repaired here since this PR edits that file. Worth knowing: `schema.json` is never loaded by `read-config.py` or any test — it is purely an editor hint, so `additionalProperties: false` is **not** runtime-enforced. A requested "schema rejects unknown keys" test was therefore not meaningful; instead a test now asserts the schema stays parseable and keeps documenting the keys the field registry actually reads. Wiring real `jsonschema` validation into `read-config.py` was considered and deliberately deferred — it would make every consumer repo's `project.yml` a CI failure risk. ## Breaking change — consumer action required `verify-command` is removed outright: no compatibility shim, no deprecation path. `cuioss/plan-marshall` is the only consumer and **must bump its SHA pin** (`@<sha> # v<version>`) for this release, and migrate any `pyprojectx.verify-command` in its `.github/project.yml` to `verify-goals` / `verify-args`. This release changes the reusable's input surface, so pinned consumers are unaffected until they bump. ## Verification - `./pw verify workflow` — 215 passed (mypy + ruff + pytest) - Workflow YAML parses - Goal allowlist rejects `verify; rm -rf /`, `$(id)`, `./pw verify`, `Verify`, `-x` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Verification workflows now support running multiple ordered Pyprojectx goals. * Added shared arguments that are appended to each verification goal. * Verification goals are validated before execution and stop on the first failure. * Pull request verification runs now cancel superseded in-progress runs. * **Documentation** * Updated configuration references, workflow guidance, schema documentation, and examples for the new verification options. * **Tests** * Added coverage for defaults, multiple goals, argument handling, ordering, and schema validation. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --- ## Review round 1 — two real defects fixed (2nd commit) **Input precedence was broken (CodeRabbit, critical).** The registry default for `verify-goals` was `'verify'`, so the action output was always non-empty and `config.outputs.X || inputs.X` always short-circuited to the config side — a caller's `verify-goals` input could never be honored. Both keys now default to `""` (as `python-version` already does), restoring `project.yml` > input > workflow-default. Effective behavior for a caller passing nothing is unchanged. **GITHUB_OUTPUT forging (Gemini, high).** Values land as `key=value` lines, so a newline in `project.yml` could emit a second line and override an unrelated output. Goals now have whitespace collapsed (injected text becomes a token the workflow's allowlist rejects loudly); args are dropped wholesale if any token is unsafe, rather than partially stripped. Shell injection was already blocked by the allowlist + `env` passing.⚠️ **Pre-existing, not fixed here:** the same precedence bug affects `cache-dependency-glob` (registry default `uv.lock`) and `upload-artifacts-on-failure` (`false`) — caller inputs for those are silently ignored today. Out of scope for this PR; worth a follow-up. --------- Co-authored-by: Oliver Wolff <23139298+cuioss@users.noreply.github.com> Co-authored-by: Claude <noreply@anthropic.com>
cuioss-oliver
force-pushed
the
feature/marshalld-interaction-audit-logging
branch
from
July 20, 2026 12:22
596e404 to
b44a2b2
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/plan-marshall/build-server/test_marshalld_audit.py (1)
184-202: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winGC window tests never exercise the real
now_utc_iso().Both timestamps here are produced via
monkeypatch.setattr(audit_mod, 'now_utc_iso', lambda: '...'), so this test (and every other test in the file) only proves_parse_iso_epoch/_is_expiredwork against a hand-crafted string — it never proves the real, unmockednow_utc_iso()fromfile_opsactually round-trips through_parse_iso_epoch. Givengc()'s correctness silently degrades to "never prunes anything" on a format mismatch (see companion comment on_marshalld_audit.py), consider adding one unmocked test asserting_parse_iso_epoch(now_utc_iso()) is not None.Suggested additional test
def test_now_utc_iso_round_trips_through_parse_iso_epoch(): """Guards the (currently convention-only) format contract between file_ops.now_utc_iso() and _marshalld_audit._parse_iso_epoch.""" assert audit_mod._parse_iso_epoch(audit_mod.now_utc_iso()) is not None🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/plan-marshall/build-server/test_marshalld_audit.py` around lines 184 - 202, Add an unmocked test near test_gc_removes_only_records_past_the_window that calls audit_mod.now_utc_iso() and asserts audit_mod._parse_iso_epoch(...) is not None, preserving the existing mocked timestamp tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@test/plan-marshall/build-server/test_marshalld_audit.py`:
- Around line 184-202: Add an unmocked test near
test_gc_removes_only_records_past_the_window that calls audit_mod.now_utc_iso()
and asserts audit_mod._parse_iso_epoch(...) is not None, preserving the existing
mocked timestamp tests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: cuioss/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 54ff457c-643c-40f8-873d-90d90bd952a6
⛔ Files ignored due to path filters (1)
pw.lockis excluded by!**/*.lock
📒 Files selected for processing (16)
marketplace/bundles/plan-marshall/skills/build-server-client/SKILL.mdmarketplace/bundles/plan-marshall/skills/build-server-client/scripts/build_server.pymarketplace/bundles/plan-marshall/skills/manage-build-server/SKILL.mdmarketplace/bundles/plan-marshall/skills/manage-build-server/scripts/_marshalld_audit.pymarketplace/bundles/plan-marshall/skills/manage-build-server/scripts/manage_build_server.pymarketplace/bundles/plan-marshall/skills/manage-build-server/scripts/marshalld.pymarketplace/bundles/plan-marshall/skills/manage-providers/scripts/_list_providers.pymarketplace/bundles/pm-plugin-development/skills/plugin-doctor/scripts/_analyze_markdown.pypyproject.tomltest/plan-marshall/build-server/test_build_server_client.pytest/plan-marshall/build-server/test_interaction_audit_correlation.pytest/plan-marshall/build-server/test_manage_build_server.pytest/plan-marshall/build-server/test_marshalld_audit.pytest/plan-marshall/script-shared/test_build_execute_factory.pytest/plan-marshall/script-shared/test_build_queue_slot.pytest/plan-marshall/workflow-integration-github/test_re_review_strategy.py
🚧 Files skipped from review as they are similar to previous changes (6)
- test/plan-marshall/script-shared/test_build_execute_factory.py
- marketplace/bundles/plan-marshall/skills/build-server-client/SKILL.md
- marketplace/bundles/plan-marshall/skills/manage-build-server/scripts/manage_build_server.py
- test/plan-marshall/build-server/test_manage_build_server.py
- marketplace/bundles/plan-marshall/skills/build-server-client/scripts/build_server.py
- marketplace/bundles/plan-marshall/skills/manage-build-server/scripts/marshalld.py
cuioss-oliver
added a commit
that referenced
this pull request
Jul 20, 2026
…best-effort attribution, gc format guard Three valid findings from the PR #949 review, all against this plan's own diff: - CWE-117 (Major): `run_wait` interpolated the client-supplied `--job-id` into the line-oriented work log unsanitized, so a `\n`/`\r` in job_id could forge a fake header line — the same vector already closed for the executor notation. Extract a single `_sanitize_for_log` seam (notation reuses it) and apply it to job_id. - Stability (Major): `_audit_interaction` derived attribution (`_audit_attribution` → `canonicalize_root` → `Path.resolve()`, which can raise `OSError` on a symlink loop) OUTSIDE the best-effort try. A raise there propagated out of `handle_request` and aborted an otherwise-successful request, contradicting the "a disk failure must never abort request handling" contract. Move the whole derivation inside the guard and broaden it so the entire audit path is best-effort. - Test guard (Trivial): every gc test mocked `now_utc_iso`, so none proved the real unmocked output round-trips through `_parse_iso_epoch` — a format drift would silently degrade gc() to "never prunes". Add one unmocked round-trip assertion. Regression tests added for all three: a wait-path control-char job_id header-forge guard mirroring the submit-path one, a direct sync `_audit_interaction` attribution-failure-is-swallowed test, and the unmocked timestamp round-trip. Whole-tree `./pw verify` green (135s). Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
marketplace/bundles/plan-marshall/skills/manage-build-server/scripts/marshalld.py (1)
456-468: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winWrap startup GC in a best-effort guard
InteractionAudit.gc()already tolerates unreadable log contents, but the prune path still rewrites the file and can raiseOSErrorfromatomic_write_file()orchmod(). A cleanup failure here can still stop daemon startup, so suppressOSErroraround this call or handle it insidegc().🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@marketplace/bundles/plan-marshall/skills/manage-build-server/scripts/marshalld.py` around lines 456 - 468, Wrap the self._interaction_audit.gc() call in serve() with best-effort OSError handling so failures from atomic_write_file() or chmod() do not abort daemon startup. Preserve the existing startup sequence and continue serving when cleanup fails.
🧹 Nitpick comments (1)
marketplace/bundles/plan-marshall/skills/manage-build-server/scripts/marshalld.py (1)
302-330: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win
reasoncapture misses theerrorkey used by most failure responses.
unknown_op,invalid_job, andmissing_jobresponses set anerrorkey, notreason(onlystatus_payload(STATUS_REFUSED, reason=...)usesreason). So for these common failure paths the audit record getsoutcome='error'withreason=None, losing the actual failure detail in the audit trail.♻️ Proposed fix
- reason = response.get('reason') + reason = response.get('reason') or response.get('error')🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@marketplace/bundles/plan-marshall/skills/manage-build-server/scripts/marshalld.py` around lines 302 - 330, The _audit_interaction method currently captures only response['reason'], so failure responses using the error key lose their detail. Update reason extraction in _audit_interaction to use the response’s error value when reason is absent, while preserving the existing reason value for responses that provide it; continue passing the resulting text to InteractionAudit.record.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In
`@marketplace/bundles/plan-marshall/skills/manage-build-server/scripts/marshalld.py`:
- Around line 456-468: Wrap the self._interaction_audit.gc() call in serve()
with best-effort OSError handling so failures from atomic_write_file() or
chmod() do not abort daemon startup. Preserve the existing startup sequence and
continue serving when cleanup fails.
---
Nitpick comments:
In
`@marketplace/bundles/plan-marshall/skills/manage-build-server/scripts/marshalld.py`:
- Around line 302-330: The _audit_interaction method currently captures only
response['reason'], so failure responses using the error key lose their detail.
Update reason extraction in _audit_interaction to use the response’s error value
when reason is absent, while preserving the existing reason value for responses
that provide it; continue passing the resulting text to InteractionAudit.record.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: cuioss/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 75830ba6-10b2-4714-aa50-bbbbb13dcd66
📒 Files selected for processing (5)
marketplace/bundles/plan-marshall/skills/build-server-client/scripts/build_server.pymarketplace/bundles/plan-marshall/skills/manage-build-server/scripts/marshalld.pytest/plan-marshall/build-server/test_build_server_client.pytest/plan-marshall/build-server/test_interaction_audit_correlation.pytest/plan-marshall/build-server/test_marshalld_audit.py
🚧 Files skipped from review as they are similar to previous changes (2)
- test/plan-marshall/build-server/test_interaction_audit_correlation.py
- marketplace/bundles/plan-marshall/skills/build-server-client/scripts/build_server.py
…job_id-correlated, secrets-safe)
… three audit stores
…e marshalld daemon The build-execute routing seam (_build_execute_factory.cmd_run) routes a build to marshalld when the project is registered AND the daemon is ready, BEFORE reaching the in-process build-queue-slot path. The queue-slot and factory cmd_run tests monkeypatch only the in-process path, so on a host where the meta-project is registered and the daemon is up (as of the PLAN-02 registration-scope work), cmd_run routed the build to the real daemon and the injected exec recorder / queue double were never exercised — 8 tests failed with exec_recorder.ran==False, empty sleeps, and rc==0 instead of 1. Force the D5 routing seam to signal fallback (_route_to_daemon -> (None, 'no_notation')) in both test setups so these tests exercise the in-process path they are written for, regardless of daemon state on the host. Pre-existing hermeticity gap exposed by the live daemon; green in daemon-free CI. Fixed as a Boy-Scout scope deviation while shipping the interaction-audit-logging plan. Co-Authored-By: Claude <noreply@anthropic.com>
…dable log Pre-submission self-review caught an unguarded boundary: InteractionAudit.read_all() called read_text() with no guard, and gc() (which calls read_all) runs unguarded from Daemon.serve() at daemon startup. A corrupted or non-UTF-8 interaction-audit.log would raise OSError/UnicodeDecodeError and crash the whole daemon at startup — inconsistent with the sibling rotate_log() call in the same serve() window, which is explicitly best-effort (try/except OSError: pass). The one other caller, run_logs, guarded only with except OSError, which does not catch UnicodeDecodeError. Harden read_all() at the single read choke point to fail closed (return []) on both OSError and UnicodeDecodeError, so gc() and run_logs degrade to an empty view rather than crashing. Remove the now-dead except OSError guard in run_logs. Add regression tests asserting read_all() and gc() tolerate a non-UTF-8 corrupt log without raising. Co-Authored-By: Claude <noreply@anthropic.com>
…ion-audit log Follow-up to the read_all hardening: removing run_logs' dead except OSError guard left contract drift — run_logs' docstring still promised a log_unreadable reason that was no longer emitted, so a present-but-corrupt log silently returned an unlabelled empty view (self-review finding 14d972). Add InteractionAudit.read_records_or_none() as the single read+parse choke point: it returns None for a present-but-unreadable log (OSError or UnicodeDecodeError), [] for absent or present-and-empty, and the parsed records otherwise. read_all() delegates and collapses None to [] so gc()/daemon-startup stay crash-safe, while run_logs keys its explicit log_unreadable reason off the None — restoring the D3 fail-closed-with-named-reason success criterion. Add tests for the log_unreadable run_logs path and the three-state read_records_or_none contract. Co-Authored-By: Claude <noreply@anthropic.com>
Self-review finding 261e0c: the `logs --limit` help text ("newest tail") and the
manage-build-server SKILL.md ("newest-first tail") implied the returned records
are newest-first, but read_all() returns oldest-first append order and run_logs'
tail=scoped[-limit:] preserves it, so records[0] is the oldest of the window.
Reword both to state "the N most recent records, ordered oldest-first within the
returned window" so a caller cannot misread records[0] as the most recent.
Co-Authored-By: Claude <noreply@anthropic.com>
…n-audit-logging Proactive finalize security-audit found two hardening opportunities in the new interaction-audit-logging surface: - CWE-117 log injection (6e0f85): build_server.py derived the audit-log notation directly from client-controlled command[2] and interpolated it into a free-text, line-oriented work-log message (not JSON-escaped, parsed back by a per-line header regex). An embedded newline could forge a fake log-entry header into the plan work log, reachable even on the degraded handshake-failure path. Strip C0 control chars from the notation before it is used for logging/ledger correlation; the daemon still receives the original unmodified command, so allowlist verification is unaffected. - Secrets-discipline backstop (f62d59): InteractionAudit.record enforced its no-secrets contract only by docstring convention. Add a closed _FORBIDDEN_EXTRA_KEYS set (command/args/argv/spec/env/cwd/exec_path/project_path) that raises ValueError immediately, so a future caller cannot silently leak a secret-shaped field into the durable audit trail (the plan's hard S4 no-secrets requirement). Regression tests added for the notation sanitizer, the end-to-end forged-header path, and every forbidden extra key. Co-Authored-By: Claude <noreply@anthropic.com>
…op nested event loop The correlation test drove the daemon by monkeypatching the client's _call_daemon to `asyncio.run(daemon.handle_request(...))` — nesting a fresh event loop inside a synchronous client callback per call, for no benefit. Replace it with the sibling daemon tests' proven-safe shape: one top-level asyncio.run of the daemon dispatch to obtain the job_id + server audit record, then feed that response back through a canned client stub so the client work log and change-ledger row carry the same job_id. Same three-store correlation assertions, no nested loops. Co-Authored-By: Claude <noreply@anthropic.com>
…1 time-bomb test_main_re_review_wires_args_and_prints_toon and test_main_re_review_accepts_sourcery_bot_kind exercise the real cmd_re_review -> request_fresh_review path, which stamps trigger_time via _now_iso() (the real wall clock). _match_review only accepts a review whose submitted_at post-dates that trigger, and both tests hardcode a review at 2026-01-01T00:05:00Z without freezing _now_iso. While the wall clock was before Jan 2026 the review post-dated "now" and matched instantly; once the clock passed 2026-01-01 the match can never succeed, so poll_until busy-loops on CPU (these tests no-op time.sleep) for the full DEFAULT_CI_TIMEOUT before passing via the timed-out branch. That CPU spin is survivable on a many-core dev machine but starves a 2-core CI runner — the observed CI slowdown/"hang", surfaced by pytest-timeout as a >300s failure. Freeze github_re_review._now_iso to 2026-01-01T00:00:00Z in both tests so the trigger deterministically precedes the mocked review's 00:05 stamp and the match is instant. Production code is unchanged and correct — a real trigger IS "now"; only the tests needed to pin the clock. Co-Authored-By: Claude <noreply@anthropic.com>
…gate, fix latent mypy findings
Add pytest-timeout (timeout=300, default signal method) so a hung/busy-looping
test surfaces as an attributable >300s failure with a stack instead of an opaque
CI job timeout. Adding it re-resolved the previously all-unpinned [tool.pyprojectx.main]
env, which floated every tool to a new major and broke the gate on unrelated code:
- mypy 1.x -> 2.3.0 flagged pre-existing latent type errors (index/return-type) in
_analyze_markdown.py and _list_providers.py.
- pytest 8 -> 9 and pytest-xdist -> 3.8 crashed xdist workers ("node down") and
then loadscope internal-errored (KeyError <WorkerController>), aborting the run.
Cap each below its next major (mypy<2, pytest<9, pytest-xdist<3.8) so adding a dev
tool can no longer silently drag the type-checker or test runner across a major
version — adopting those majors is a deliberate migration, not a transitive
side effect. pw.lock captures the resolved versions for a reproducible env.
Also fix the two latent findings the newer mypy correctly caught (annotate the
widened dict unions as dict[str, Any]) so the files are clean under 1.x and the
eventual 2.x migration alike.
Co-Authored-By: Claude <noreply@anthropic.com>
… CI (asyncio subprocess-cancel) The correlation test drove the daemon via asyncio.run(daemon.handle_request(submit)), assuming Scheduler(max_slots=0) would keep the job queued with no subprocess. It does not: Scheduler clamps to max(1, max_slots), so the job is admitted, _execute spawns a real run_job subprocess as a fire-and-forget task, and asyncio.run then hangs at loop-close cancelling that in-flight subprocess transport. The hang is Python-version specific — reproduced under CI's 3.12 (macOS/Linux kqueue/epoll select(timeout=None)), invisible under local 3.14 — which is why every local run was green while CI wedged for ~90 min until pytest-timeout finally surfaced it as a >300s failure. Drive the two seams the correlation test actually needs — daemon job_id assignment (_scheduler.submit, enqueue only) and the audit-on-dispatch write (_audit_interaction) — synchronously, with no event loop, no admission, and no subprocess. Verified under Python 3.12.3 (the CI interpreter): 3 passed in 0.10s, previously hung to timeout. Co-Authored-By: Claude <noreply@anthropic.com>
…best-effort attribution, gc format guard Three valid findings from the PR #949 review, all against this plan's own diff: - CWE-117 (Major): `run_wait` interpolated the client-supplied `--job-id` into the line-oriented work log unsanitized, so a `\n`/`\r` in job_id could forge a fake header line — the same vector already closed for the executor notation. Extract a single `_sanitize_for_log` seam (notation reuses it) and apply it to job_id. - Stability (Major): `_audit_interaction` derived attribution (`_audit_attribution` → `canonicalize_root` → `Path.resolve()`, which can raise `OSError` on a symlink loop) OUTSIDE the best-effort try. A raise there propagated out of `handle_request` and aborted an otherwise-successful request, contradicting the "a disk failure must never abort request handling" contract. Move the whole derivation inside the guard and broaden it so the entire audit path is best-effort. - Test guard (Trivial): every gc test mocked `now_utc_iso`, so none proved the real unmocked output round-trips through `_parse_iso_epoch` — a format drift would silently degrade gc() to "never prunes". Add one unmocked round-trip assertion. Regression tests added for all three: a wait-path control-char job_id header-forge guard mirroring the submit-path one, a direct sync `_audit_interaction` attribution-failure-is-swallowed test, and the unmocked timestamp round-trip. Whole-tree `./pw verify` green (135s). Co-Authored-By: Claude <noreply@anthropic.com>
cuioss-oliver
force-pushed
the
feature/marshalld-interaction-audit-logging
branch
from
July 20, 2026 18:29
287b13d to
279aa85
Compare
…raction audit Two valid findings from CodeRabbit's review of the prior loop-back commit (287b13d), both in marshalld.py: - Major: Daemon.serve() called self._interaction_audit.gc() unguarded. gc()'s prune path rewrites the log via atomic_write_file()/chmod(), either of which can raise OSError; an unguarded raise there aborts daemon startup. Wrapped in a best-effort OSError guard so a cleanup failure carries the stale records to the next start's GC instead of taking the daemon down — same posture as the existing _audit_interaction best-effort guard and the sibling rotate_log startup guard. - Trivial: _audit_interaction captured only response['reason'], but the unknown_op / invalid_job / missing_job failure responses set `error`, not `reason` — so those records logged outcome='error' with reason=None, losing the failure detail. reason now falls back to response['error']. Regression test asserts the unknown-op audit record keeps its error detail. Whole-tree ./pw verify green (147s). Co-Authored-By: Claude <noreply@anthropic.com>
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.
Summary
Add full interaction audit logging to the
marshalldbuild server, on both sides of the client/server boundary, correlated end-to-end via the existingjob_id.build-server-client): every daemon interaction (submit/enqueue, status/waitquery, fallback/refused(reason=...)outcome) is now logged to the plan work log viamanage-logging— no interaction is silent, and no fallback/refusal lands only at an uncaptured Python log level.marshalld.py): a structured, append-only interaction log records every request/response, stamped withproject_root+plan_id+job_id+timestamp+outcome, extending the existingjob-logs/directory rather than duplicating it.job_idalready written to the change-ledgerkind=jobentry at submit time ties the client work-log entries, the server's interaction-log records, and the ledger entry together end-to-end.manage-build-serverread verb inspects recorded interactions (status/logs shape).Changes
marketplace/bundles/plan-marshall/skills/build-server-client/scripts/build_server.py— client-side interaction logging for submit/status/wait/fallback/refusal pathsmarketplace/bundles/plan-marshall/skills/build-server-client/SKILL.md— documents the client-side audit-logging contractmarketplace/bundles/plan-marshall/skills/manage-build-server/scripts/marshalld.py— server-side structured interaction log wiringmarketplace/bundles/plan-marshall/skills/manage-build-server/scripts/_marshalld_audit.py— new interaction-log module (append-only records, retention/GC)marketplace/bundles/plan-marshall/skills/manage-build-server/scripts/manage_build_server.py— new read verb to inspect recorded interactionsmarketplace/bundles/plan-marshall/skills/manage-build-server/SKILL.md— documents the audit read verb and log structuretest/plan-marshall/build-server/test_build_server_client.py— client-side logging assertionstest/plan-marshall/build-server/test_interaction_audit_correlation.py— new correlation-id test across client/server/ledgertest/plan-marshall/build-server/test_manage_build_server.py— read-verb coveragetest/plan-marshall/build-server/test_marshalld_audit.py— new interaction-log module teststest/plan-marshall/script-shared/test_build_execute_factory.py,test/plan-marshall/script-shared/test_build_queue_slot.py— adjusted shared test fixturesTest Plan
python3 .plan/execute-script.py plan-marshall:build-pyproject:pyproject_build run --command-args "verify")Related Issues
None
Summary by CodeRabbit
logscommand to inspect recent interaction audit records.logsfails closed with explicit empty results when the audit log is missing or unreadable.