Resolve artifact bucket from merged tree, not the untrusted PR head - #152
Conversation
The controller read S3_ARTIFACT_BUCKET from the PR head tree before the ephemeral merge. A PR head lacking the project's private settings override (e.g. an upstream-sync branch) resolved the wrong bucket, so the snapshot upload hit AccessDenied (IAM correctly refused the off-scope write). Fix: in force-merge mode read the bucket from the merged tree (base + head) after the merge, the same tree the reinstalled runtime uses, and disable the sticky merge base so nothing needs the bucket before the merge. - merge.read_repo_settings: force_merge_commit now also forces STICKY_MERGE_BASE_HOURS to 0. - merge.prepare_repo_snapshot: re-read the bucket from the merged clone and publish to it. - Document the sticky-base limitation in docs/ci-config.md. - Bump praktika 0.1.13 to 0.1.14, praktika-controller 0.1.7 to 0.1.8. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Workflow [Praktika CI Advanced], commit [4b49a42] Summary: ✅ Code ReviewResult: ❌ Blocking issues What changed: Adds SSM-backed Praktika/controller version pinning, controller self-updates, JSONC config support, and runtime venv cache handling. It also changes snapshot creation/upload and merged-tree bucket resolution, improves workflow-load failure reporting, and adds Docker proxy and infrastructure deployment enhancements. Blocking issues
The previously reported self-update result handling, mutable local-runtime cache, and workflow-load check issues are fixed in the current code. Investigation: 6/13 rounds, 36 tool calls. |
…toml ve Update _HEAD_PRAKTIKA_VERSION to match the current pyproject.toml version (0.1.14) so the Version Check job's assertion passes. AI-Session-Round: 9bba16ab004c
![praktika-gh[bot]](https://avatars.githubusercontent.com/in/3494315?s=60&v=4){kind=small}
60f2bdf to
e9df0c2
Compare
| MAX_ATTEMPTS, | ||
| ) | ||
| try: | ||
| _pip_install(desired_source, python=python, run=run) |
There was a problem hiding this comment.
Blocking: The documented filesystem/repo-path form cannot work here. maybe_self_update runs immediately after receiving the message, before the workflow repository is cloned, and _pip_install passes a relative source such as . or ./bootstrap directly to pip. It therefore resolves against the systemd process's working directory rather than the run checkout, repeatedly fails, and eventually processes the message under the old controller after hitting the attempt cap. Either resolve/prepare the checkout before installation or restrict controller pins to version specs, immutable URLs, and absolute host paths.
There was a problem hiding this comment.
Fixed in fa7e22d. Controller pins are now validated up front (_is_valid_controller_source): only a URL, a requirement spec, or an absolute host path are accepted. A relative path (., ./bootstrap) is rejected cleanly with an error log and no install attempt (so it never burns the attempt cap), since self-update runs before any checkout exists. Documented in ci-config.md; praktika_version still resolves relative paths against the run checkout as before. Added test_relative_path_pin_is_rejected_without_install.
- Read ci_config once per received message and pass the snapshot into handle_workflow, so the controller self-update decision and the value frozen into run metadata come from the same read (no split on a mid-message SSM change). - Run controller self-update inside a VisibilityHeartbeat and release the message only after the heartbeat stops, so a slow download+pip install cannot let the message be picked up concurrently. - Give the fresh-instance self-update a best-effort rollback target (the running version by spec); note the in-place install atomicity gap as deferred production hardening. - Key ensure_praktika_venv's cache by the install source so a changed praktika_version pin actually reinstalls instead of reusing a stale venv. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Controller self-update runs before any workflow checkout exists, so a
relative repo-path praktika_controller_version (".", "./bootstrap") can't
be resolved and would loop until the attempt cap. Restrict controller
pins to version specs, URLs, or absolute host paths and reject a relative
path cleanly (no install attempt). praktika_version keeps resolving
relative paths against the run checkout as before.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
| pin = _controller_version_pin(ci_config) | ||
| if pin: | ||
| with VisibilityHeartbeat(sqs, queue_url, receipt, visibility): | ||
| did_self_update = maybe_self_update(pin, log) |
There was a problem hiding this comment.
Blocking: False from maybe_self_update does not only mean “already at the pin”; it also means an invalid source, an exhausted attempt cap, an install failure, or a failed import/rollback. Every one of those cases falls through and processes this message under the old controller, despite its frozen praktika_controller_version. A bad or temporarily unavailable pin can therefore split one run across controller versions. Return a distinct failure state (or raise) and release/fail the message without processing it; continue only when the desired source is confirmed satisfied.
load_ci_config now strips // and /* */ comments and trailing commas
before json.loads, so an operator can comment out a field without a
strict parse error silently reading the whole parameter as {} (every
feature off). The stripper is string-aware: a // inside an https:// URL
or a comma inside a string value is preserved.
Also lock in that an empty-string version pin is treated as unset.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
deploy() now seeds the out-of-repo CI config parameter with a
commented-out JSONC template (parses to {}, every feature off) that
documents all supported keys with short descriptions. Create-if-absent
only (ParameterAlreadyExists is ignored) so a redeploy never clobbers an
operator's live value. Gated by _wants("CIConfig"), so a full deploy or
`--only CIConfig` creates it. Destroy already sweeps it by prefix.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The base workflow-orchestrator runs the AMI-baked praktika (pinned to _PRAKTIKA_BASE_VERSION), while the repo's workflow files can move ahead. One file using a newer feature (e.g. ci/workflows/ignition_dispatch.py's Workflow.Engine.GH_IGNITION, absent in 0.1.9) raised AttributeError at import inside _get_workflows(), crashing the whole scan -> orchestrate rc=100 -> InfraOrchestrationError -> the workflow never started and the "CI" check was left stuck in_progress. 1) Bump _PRAKTIKA_BASE_VERSION 0.1.9 -> 0.1.13 (latest published wheel that has GH_IGNITION; 0.1.14 is not published yet). Requires a base AMI rebake + base ASG roll. 2) Harden _get_workflows(): a single workflow file that fails to import is skipped with a warning instead of crashing the scan, so the other workflows still load and match. Validation (_for_validation_check) re-raises so `praktika validate`, run under the checkout's own praktika, still catches genuinely broken files. 3) Surface skipped files in GitHub checks: load errors are threaded out of _get_workflows -> find_workflows_for_event -> the orchestrator, which posts a completed "Workflow load error: <file>" failure check on the PR head so a skipped workflow is never silently lost. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…tity r4143720081: when EVERY workflow file fails to import, _get_workflows() previously hit its `if not res: raise` before returning, so load errors were never surfaced and the bootstrap check was left in_progress. Now it returns empty when load errors were collected; the orchestrator posts the per-file "Workflow load error" checks and finalizes the bootstrap check as failure instead of neutral. r4143720089: self-update verification only proved *some* praktika-controller imports, not that desired_source installed the intended distribution. A typo like `praktika==0.1.8` or a URL to an unrelated wheel would install, read back the old version, and be persisted as converged. Now the pin's project identity is validated before install (from the == spec name or the wheel filename) and, for an exact pin, the installed version must match after install before it is adopted. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
e653a62 to
a43819d
Compare
The "already converged" and "already at pinned version" short-circuits returned silently, so a matching praktika_controller_version pin looked like nothing happened (no log line between RECEIVED and Processing). Log both at INFO so an operator can confirm the pin was read and honored. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Per PR review r4143720089: this is a dev-mode mechanism, so drop the identity/version validation, post-install verification, rollback, and crash-loop counter. maybe_self_update now: no-op on empty/already-pinned source (persisted), else pip install --force-reinstall and restart. A bad/unreachable pin fails hard (pip's error propagates) instead of being validated or recovered. State keeps only installed_source. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Per PR review r4144394537: keying the no-base_venv cache by the source string still served stale Praktika for a mutable repo/filesystem pin (e.g. praktika_version: "."), because the same path with different content across runs computes the same key and the cached venv is reused as soon as `import praktika` succeeds. Now a cached venv is reused as-is only for passthrough (immutable) sources — a URL or version spec; for a local-path source it is reinstalled with --force-reinstall --no-deps on every task, mirroring the base-venv overlay, so a run never executes a stale checkout. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Per PR review r4144514665: CheckRun (orchestrator/check_run.py) has no
create_completed() — that helper is on state.py's check class — so the
call raised AttributeError, swallowed by the surrounding except, and the
failure check was never posted (a sibling workflow's bootstrap check could
even go green). Use the existing two-call API: start(... with_cancel_action
=False).complete("failure", output=...). Added a regression test that the
load-error check is actually posted.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
| log.info("Self-updating controller -> %s", desired_source) | ||
| _pip_install(desired_source, python=python, run=run) # fail hard on a bad pin | ||
| state["installed_source"] = desired_source | ||
| _save_state(state, log) |
There was a problem hiding this comment.
Blocking: _save_state swallows every write failure, but this still returns True and restarts the process. On restart, the old or missing state again differs from desired_source, so the controller reinstalls, releases the message, and restarts forever. Because each iteration exits cleanly, the poll loop's infra-failure replacement path is never reached. Persist the converged source durably before signaling restart, and propagate a persistence failure so the instance is retried/replaced.
There was a problem hiding this comment.
_save_state is in the dev mode branch. acceptable
| # controller self-update decision here and (for a workflow) the value | ||
| # handle_workflow freezes into run metadata — never re-read SSM, or a | ||
| # mid-message change could split the two. | ||
| ci_config = _ci_config_for_message(role, payload, log) |
There was a problem hiding this comment.
Blocking: load_ci_config converts every SSM failure—including timeouts, access errors, and malformed values—to {}. This call therefore interprets a transient failure as an explicit absence of the controller/runtime pins and processes the workflow under whatever versions happen to be installed, freezing the empty config into all job tasks. That defeats the pin precisely when SSM is temporarily unavailable. Treat only a genuinely missing optional parameter as {}; operational/read/parse failures should abort this message and enter the normal infra-retry path.
1 participant
Uh oh!
There was an error while loading. Please reload this page.